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

build: switch realpath to pwd #31095

Closed
wants to merge 3 commits into from
Closed

build: switch realpath to pwd #31095

wants to merge 3 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Dec 26, 2019

Our test coverage linux machine doesn't have the realpath bin, which I believe comes in as part of coreutils?

This became an issue in #30954, which I believe caused realpath to start getting invoked on these machines for the first time.

I believe we can simply swap out the use of realpath to pwd, for our purposes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 26, 2019
@Trott
Copy link
Member

Trott commented Dec 26, 2019

Our test coverage linux machine doesn't have the realpath bin, which I believe comes in as part of coreutils?

@nodejs/build

@Trott
Copy link
Member

Trott commented Dec 26, 2019

I believe we can simply swap out the use of realpath to pwd, for our purposes.

@nodejs/python @nodejs/build-files

Copy link
Member

@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.

Do we need the prefix at all? Other parts of node.gypi also refer to source tree files using relative paths, so I’d expect that to work too

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@bcoe
Copy link
Contributor Author

bcoe commented Dec 26, 2019

@addaleax having experimented with our CI, it does seem that the path provided to the linker needs to be absolute; I've reverted to the prior pwd approach, and it seems to be working like a charm. I think pwd will be more universally available out of the gate, than realpath.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

Maybe fast track this to fix the coverage builds?

@gabrielschulhof gabrielschulhof added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 27, 2019
gabrielschulhof pushed a commit that referenced this pull request Dec 27, 2019
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gabrielschulhof
Copy link
Contributor

Landed in ce0fb0f.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 31, 2019
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins pushed a commit that referenced this pull request Jan 8, 2020
Backport-PR-URL: #31063
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Backport-PR-URL: #31063
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 9, 2020
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Backport-PR-URL: nodejs#32092
PR-URL: nodejs#31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
Backport-PR-URL: #32092
PR-URL: #31095
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants