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

tools,gyp: don't force build actions with multiple outputs #23982

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 30, 2018

Excluding macOS which has a different way of resolving dependencies which can lead to deadlocks.

Reroll: #23156
Refs: #23257
Fixes: #23255

Don't add force_append (FORCE_DO_CMD) to the intermediate sentinel.
Adding it makes the action run alway, even when there are no changes.
(refack): AFAICT because *.intermediate files don't have build rules.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Don't add `force_append` (FORCE_DO_CMD) to the intermediate sentinal.
Adding it makes the action run alway, even when there are no changes.

Excluding macOS which has a different way of resolving dependancies
which can lead to deadlocks.

(refack): AFAICT because `*.intermediate` files don't have build rules.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Oct 30, 2018
@refack
Copy link
Contributor Author

refack commented Oct 30, 2018

/CC @nodejs/python @nodejs/build-files @nodejs/gyp

@refack refack added the macos Issues and PRs related to the macOS platform / OSX. label Oct 30, 2018
if self.flavor == 'mac':
self.WriteLn('%s: %s%s' % (intermediate, ' '.join(inputs), force_append))
else:
self.WriteLn('%s: %s' % (intermediate, ' '.join(inputs)))
Copy link
Member

Choose a reason for hiding this comment

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

This is the wrong way of doing it. You're effectively ignoring the value of the force argument to WriteMakeRule() here.

The call comes from WriteDoCmd(). Forcing the action to run when it's a command is correct; commands are imperatives. You should try to come up with another way of solving this.

Copy link
Contributor Author

@refack refack Oct 31, 2018

Choose a reason for hiding this comment

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

Yeah, I know this is a hack. But the soup GYP generates for make is hard to reason about, and empirically this works (it also works on macOS, but might lead to deadlocks when run -jN with N > 1). Works as in rebuilds iff action's file inputs are touched.

I'll give it more thought...

@sam-github
Copy link
Contributor

make -j4 node, after node has just been built (so there is nothing to do), currently relinks node and cctest, and takes over half a minute. That's pretty painful. Will this fix that problem?

@targos
Copy link
Member

targos commented Dec 19, 2018

@sam-github yes. I work with this patch applied:

$ make -j4 node
make -C out BUILDTYPE=Release V=1
make[1]: Nothing to be done for 'all'.
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi

@refack refack closed this Dec 21, 2018
@refack refack deleted the reroll-gyp-no-force-tweak branch December 21, 2018 01:09
@refack refack restored the reroll-gyp-no-force-tweak branch January 22, 2019 21:42
@refack refack deleted the reroll-gyp-no-force-tweak branch March 11, 2019 01:26
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. macos Issues and PRs related to the macOS platform / OSX. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make with -j hangs
5 participants