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

Fix make tar-headers for Linux #5978

Closed
wants to merge 1 commit into from
Closed

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Mar 31, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

build

Description of change

The make tar-headers target currently fails on Linux due to the command find $(TARNAME)/ -type l | xargs rm as the find is unable to find any links, so the rm fails with a no arguments error. The simplest solution is to simply remove this line (assuming we aren't expecting an links). If anyone thinks they can still appear, then I can do something like:

if [ `find $(TARNAME)/ -type l` ]; then
  find $(TARNAME)/ -type l | xargs rm
fi

I also removed the dependency on config.gypi, as the tar-headers target runs configure itself.

Related Issue: #4149 (see this comment)

@gibfahn
Copy link
Member Author

gibfahn commented Mar 31, 2016

@rvagg I think you did the first version of tar-headers in the Makefile, let me know if this makes sense.

@addaleax
Copy link
Member

Wouldn’t using rm -f instead of rm or appending || true to the line suffice?

@jbergstroem
Copy link
Member

@addaleax: that would be my preferred solution as well.

@Fishrock123 Fishrock123 added the build Issues and PRs related to build files or the CI. label Mar 31, 2016
@gibfahn
Copy link
Member Author

gibfahn commented Apr 1, 2016

@addaleax That makes sense. Using rm -f seems a bit cleaner that || true, and doesn't leave the rm: missing operand message in the output, but they both work.

I just wondered if anyone still expected to find links in the headers folder to be tarred up.

Edit: I updated the PR to use rm -f

@gibfahn
Copy link
Member Author

gibfahn commented Apr 15, 2016

@rvagg I'd like to get this in before Node 6 comes out. Any objections to this?

@mhdawson
Copy link
Member

LGTM given the update to rm -f

@mhdawson
Copy link
Member

CI run https://ci.nodejs.org/job/node-test-pull-request/2274/, although I don't think it will necessarily test the change.

@gibfahn
Copy link
Member Author

gibfahn commented Apr 15, 2016

Couple of CI failures on ARM (which I'm pretty sure are unrelated).

I understand this probably doesn't test the changes.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM

@mhdawson
Copy link
Member

Failures look like timeouts which from the history seem to occur regularly on the arm machines, that along with the zero effect we'd expect from the change make me comfortable the failures are unrelated.

@mhdawson mhdawson self-assigned this Apr 18, 2016
@mhdawson
Copy link
Member

Unless there are objections I'd like to land this tomorrow @rvagg, @jbergstroem any objections ?

@jbergstroem
Copy link
Member

LGTM.

@bnoordhuis
Copy link
Member

LGTM2. For robustness, $(TARBALL)-headers would ideally be .PHONY.

The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.
@gibfahn
Copy link
Member Author

gibfahn commented Apr 19, 2016

@bnoordhuis Okay, I've added it to the .PHONY list at the end of the Makefile. Let me know if this is the right place/way to do it.

@bnoordhuis
Copy link
Member

LGTM. I'll let @mhdawson land it. =)

mhdawson pushed a commit that referenced this pull request Apr 20, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson
Copy link
Member

Landed as 7fc4b31

@mhdawson mhdawson closed this Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

@mhdawson lts?

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson
Copy link
Member

LTS -> yes just added lts-watch-v4.x tag.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: nodejs#5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 3, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 6, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
The tar-headers target tries to find and delete links in the
tar folder, which fails as no links are found. Use rm -f to
avoid this.

Remove the config.gypi dependency, as the target runs configure
itself.

PR-URL: #5978
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants