Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postinstall script: support monorepos #191

Merged
merged 42 commits into from
Aug 13, 2024
Merged

postinstall script: support monorepos #191

merged 42 commits into from
Aug 13, 2024

Conversation

chgeo
Copy link
Member

@chgeo chgeo commented Aug 12, 2024

In monorepo setups cds-types might already be installed elsewhere (higher up), so find and use it as link target.

This happens in monorepo setups where `cds-types` is alreday installed.
@chgeo chgeo requested a review from daogrady as a code owner August 12, 2024 11:25
@chgeo chgeo requested a review from joergmann August 12, 2024 11:25
@chgeo chgeo force-pushed the install-monorepos branch 2 times, most recently from b53e613 to a325593 Compare August 12, 2024 13:42
@chgeo chgeo force-pushed the install-monorepos branch from a325593 to 44750e5 Compare August 12, 2024 13:53
@chgeo
Copy link
Member Author

chgeo commented Aug 12, 2024

@joergmann my new monorepo test fails on Windows with
npm verbose stack Error: EPERM: operation not permitted, lstat 'C:\Users\RUNNER~1'

Can you make any sense out of it? Does the test run for you locally?

@chgeo chgeo changed the title postinstall script finds existing cds-types installation Monorepo support for postinstall script Aug 12, 2024
@chgeo chgeo changed the title Monorepo support for postinstall script postinstall script: support monorepos Aug 12, 2024
@chgeo
Copy link
Member Author

chgeo commented Aug 13, 2024

@joergmann please make sure to have the latest non 10.8.2 installed.

@joergmann
Copy link
Contributor

@chgeo fixed, will revistit to make it more robust

@chgeo
Copy link
Member Author

chgeo commented Aug 13, 2024

@daogrady this is the PR by Joerg and me, targeting the next cds-types patch.

scripts/postinstall.js Outdated Show resolved Hide resolved
name: 'project1'
}, null, 2))
{
// const {stdout, stderr} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// const {stdout, stderr} =

{
// const {stdout, stderr} =
await execAsync(`npm i --foreground-scripts -dd -D ${cdsTypesRoot}`, { cwd: project1 })
// console.log(stdout, stderr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// console.log(stdout, stderr)

}
}, null, 2))
{
// const {stdout, stderr} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// const {stdout, stderr} =

{
// const {stdout, stderr} =
await execAsync(`npm i --foreground-scripts -dd`, { cwd: project2 })
// console.log(stdout, stderr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// console.log(stdout, stderr)

@@ -16,6 +16,7 @@ describe('postinstall', () => {

beforeEach(async () => {
tempFolder = await fs.mkdtemp(path.join(os.tmpdir(), 'postinstall-'))
// console.log(`tempFolder: ${tempFolder}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// console.log(`tempFolder: ${tempFolder}`)

@daogrady
Copy link
Contributor

Very valuable contribution, thank you both! I only added some minor nits, but I think this is good to go.

@daogrady daogrady requested review from daogrady and joergmann August 13, 2024 13:00
@chgeo chgeo merged commit 4bcc7f2 into main Aug 13, 2024
15 checks passed
@chgeo chgeo deleted the install-monorepos branch August 13, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants