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

chore: writes compiled mongosh to lib-boxednode instead of lib to avoid linking it twice when building node #43

Merged

Conversation

himanshusinghs
Copy link
Contributor

@himanshusinghs himanshusinghs commented Jul 31, 2023

As pointed by Anna, probably because of changes (1,2) in between Node16 to Node20, the compiled mongosh file that we used to write under lib dir in node source tree was getting linked twice when building Node, but one the mentioned changes made this an error state and our compilation started failing after switching to Node20.

With this change we will start writing the compiled mongosh file to lib-boxednode dir in node source tree instead of lib and will continue linking it (only once) with --link-module arg.

@himanshusinghs
Copy link
Contributor Author

Looking into test failures. Even though the the file was linked during building, the final binary is not able to require it for some reason. Works fine, if I write to source tree root and include it from there.

Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Maybe https://github.com/nodejs/node/blob/main/BUILDING.md#building-nodejs-with-external-core-modules is helpful here? i.e. the lines below

    mainSource = mainSource.replace(/\bREPLACE_WITH_ENTRY_POINT\b/g,
      JSON.stringify(`${namespace}/${namespace}`));

may need to become

    mainSource = mainSource.replace(/\bREPLACE_WITH_ENTRY_POINT\b/g,
      JSON.stringify(`/lib-boxednode/${namespace}/${namespace}`));

(and we could also drop the double namespacing here now I think, previously that was only there to avoid potential conflicts with existing files in lib/)

@himanshusinghs
Copy link
Contributor Author

That was actually it, thank you for pointing, I missed that.

@himanshusinghs himanshusinghs merged commit ecf1b1c into main Aug 1, 2023
28 of 56 checks passed
@himanshusinghs himanshusinghs deleted the MONGOSH-1536-avoid-linking-compiled-mongosh-twice branch August 1, 2023 16:31
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.

2 participants