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

Support a single NE instance per solution on Linux, too (OSOE-430) #17

Closed
0liver opened this issue Nov 4, 2022 · 11 comments · Fixed by #74 or #78
Closed

Support a single NE instance per solution on Linux, too (OSOE-430) #17

0liver opened this issue Nov 4, 2022 · 11 comments · Fixed by #74 or #78
Assignees
Labels
enhancement New feature or request

Comments

@0liver
Copy link
Contributor

0liver commented Nov 4, 2022

The OSOCE build time can be significantly reduced by using a single copy of the Node.js Extensions NPM package instead of one for every project which uses NE. This only applies to the usage via submodule, though.

The idea is to use an MSBuild property to trigger the NPM package installation as part of the Node.js Extensions project build rather than as part of the build of its dependent projects. Since all dependent projects will be built AFTER NE itself, there's no issue with concurrency or locking.

Once NE has its NPM package installed, all dependent module can simply link to that installation (using pnpm) which is a fast and cheap operation.

This will save several tens of seconds per project. Taking into account parallel builds, it should yield a speed-up of 1 to 3 minutes per build. And it does.

Linux support currently disabled

The above has been implemented in v1.0.0 of the module, but had to be deactivated for Linux builds due to the different handling of symlinked directories, which breaks the support for a single NE installation.

This should be fixed.

Jira issue

@github-actions github-actions bot changed the title Improve OSOCE build performance Improve OSOCE build performance (OSOE-430) Nov 4, 2022
@Piedone
Copy link
Member

Piedone commented Jan 18, 2023

Is this still relevant?

@0liver
Copy link
Contributor Author

0liver commented Jan 18, 2023

Yes, but I'll have to update it. Will do later.

@Piedone
Copy link
Member

Piedone commented Feb 2, 2023

Can you please update this?

@0liver 0liver changed the title Improve OSOCE build performance (OSOE-430) Support a single NE instance per solution on Linux, too (OSOE-430) Feb 2, 2023
@0liver
Copy link
Contributor Author

0liver commented Feb 2, 2023

Done.

@Piedone
Copy link
Member

Piedone commented Feb 2, 2023

Thanks!

@Piedone
Copy link
Member

Piedone commented Feb 4, 2023

Is this related to the OSOCE Linux builds taking roughly the same time as the Windows ones?

@0liver
Copy link
Contributor Author

0liver commented Feb 14, 2023

Yes.

@Piedone Piedone added the enhancement New feature or request label Mar 5, 2023
@Piedone
Copy link
Member

Piedone commented Apr 10, 2023

Does your work under #39 overlap with this?

@0liver
Copy link
Contributor Author

0liver commented Apr 11, 2023

No.

@0liver
Copy link
Contributor Author

0liver commented Jul 14, 2023

Just to put anyone picking this up on the right track:

The source of this functionality breaking on Linux is the fact that the symlink'ed directories node_modules\nodejs-extensions underneath each project are resolved to the central installation directory of NE, which will be OSOCE\node_modules.nx.

We use directory traversal via ..\.. from the NE directory into its hosting project directory, which works on Windows, because it does not resolve the symlink project\node_modules\nodejs-extensions and will end up in project, while on Linux it will start traversing from OSOCE\node_modules.nx and end up in the repo root instead of project.

I've already started to fix this issue by adding a custom script, getCwd, to NE which works around the symlink resolution on Linux, but it has to be used in all the necessary places in all the scripts, so that the build passes.

I'll leave the PRs open as they are because they already point in the right direction.

@Piedone
Copy link
Member

Piedone commented Jan 22, 2024

This is attempted again under #78.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment