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: fix dependencies on AIX #8285

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mhdawson
Member

mhdawson commented Aug 26, 2016

Checklist

  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

addons test

Description of change

addon tests were still starting to run before the node exp file
creation was complete.

  • remove process_outputs_as_sources as it did not fix the
    problem
  • update create_expfile.sh so that exp file is created in a
    temporary file and then renamed to final name so that
    file is only visible once it is complete
  • update target used in building addons so that for
    AIX it depends on the exp file being available

Fixes: #8285

build: fix dependencies on AIX
addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building addons so that for
  AIX it depends on the exp file being available
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 26, 2016

Member

Ok got only passes with about 20 runs with this patch, unless I'm even more unlucky than before it seems to stop the addon tests from running before the node.exp file is complete and ready.

Member

mhdawson commented Aug 26, 2016

Ok got only passes with about 20 runs with this patch, unless I'm even more unlucky than before it seems to stop the addon tests from running before the node.exp file is complete and ready.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
Member

mhdawson commented Aug 26, 2016

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 26, 2016

Member

CI run looks ok with only one Windows failures that looks un-related.

Member

mhdawson commented Aug 26, 2016

CI run looks ok with only one Windows failures that looks un-related.

Show outdated Hide outdated Makefile
@@ -87,7 +87,7 @@ uninstall:
$(PYTHON) tools/install.py $@ '$(DESTDIR)' '$(PREFIX)'
clean:
-rm -rf out/Makefile $(NODE_EXE) $(NODE_G_EXE) out/$(BUILDTYPE)/$(NODE_EXE)
-rm -rf out/Makefile $(NODE_EXE) $(NODE_G_EXE) out/$(BUILDTYPE)/$(NODE_EXE) out/$(BUIDLTYPE)/node.exp

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 27, 2016

Member

Typo: s/BUIDLTYPE/BUILDTYPE/

EDIT: Also, please try to stay within 80 columns.

@bnoordhuis

bnoordhuis Aug 27, 2016

Member

Typo: s/BUIDLTYPE/BUILDTYPE/

EDIT: Also, please try to stay within 80 columns.

Show outdated Hide outdated Makefile
@@ -134,10 +134,17 @@ test/gc/node_modules/weak/build/Release/weakref.node: $(NODE_EXE)
--nodedir="$(shell pwd)"
# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
test/addons/.docbuildstamp: tools/doc/addon-verify.js doc/api/addons.md
ifeq ($(OSTYPE),aix)
test/addons/.docbuildstamp: tools/doc/addon-verify.js doc/api/addons.md out/Release/node.exp

This comment has been minimized.

@bnoordhuis

bnoordhuis Aug 27, 2016

Member

Taking out a dependency on node.exp seems reasonable to me but:

  1. Don't indent the target name, and
  2. Don't duplicate the target logic, and
  3. Don't hard-code 'Release'.

You could write it like this:

DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md

ifeq ($(OSTYPE),aix)
DOCBUILDSTAMP_PREREQS = $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
endif

test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
  # ..
@bnoordhuis

bnoordhuis Aug 27, 2016

Member

Taking out a dependency on node.exp seems reasonable to me but:

  1. Don't indent the target name, and
  2. Don't duplicate the target logic, and
  3. Don't hard-code 'Release'.

You could write it like this:

DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md

ifeq ($(OSTYPE),aix)
DOCBUILDSTAMP_PREREQS = $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
endif

test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
  # ..
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 29, 2016

Member

@bnoordhuis, thanks was considering whether to combine or not but though I'd see what you thought. Have update to reflect your comments.

Member

mhdawson commented Aug 29, 2016

@bnoordhuis, thanks was considering whether to combine or not but though I'd see what you thought. Have update to reflect your comments.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 29, 2016

Member

@bnoordhuis suggestion lead to the following errors: Makefile:141: *** Recursive variable `DOCBUILDSTAMP_PREREQS' references itself (eventually). Stop.
Build step 'Execute shell' marked build as failure

Will have to look to see if there is a way around that.

Member

mhdawson commented Aug 29, 2016

@bnoordhuis suggestion lead to the following errors: Makefile:141: *** Recursive variable `DOCBUILDSTAMP_PREREQS' references itself (eventually). Stop.
Build step 'Execute shell' marked build as failure

Will have to look to see if there is a way around that.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 29, 2016

Member

@bnoordhuis ok think I fixed it with other form of assignment.

Member

mhdawson commented Aug 29, 2016

@bnoordhuis ok think I fixed it with other form of assignment.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 29, 2016

Member

LGTM but s/addon/Addon/ (or 'Add-on') in the commit log.

Member

bnoordhuis commented Aug 29, 2016

LGTM but s/addon/Addon/ (or 'Add-on') in the commit log.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 29, 2016

Member

CI run looks ok except for unrelated failures on arm.

Member

mhdawson commented Aug 29, 2016

CI run looks ok except for unrelated failures on arm.

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 29, 2016

Member

Failures look unrelated LGTM with @bnoordhuis's note

Member

MylesBorins commented Aug 29, 2016

Failures look unrelated LGTM with @bnoordhuis's note

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Aug 29, 2016

Member

Landed as 1657f12

Member

mhdawson commented Aug 29, 2016

Landed as 1657f12

mhdawson added a commit that referenced this pull request Aug 29, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

@mhdawson mhdawson closed this Aug 29, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: nodejs#8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@mhdawson I've backported this to v4.x, please let me know if it shouldn't be

Member

MylesBorins commented Sep 30, 2016

@mhdawson I've backported this to v4.x, please let me know if it shouldn't be

MylesBorins added a commit that referenced this pull request Sep 30, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

build: fix dependencies on AIX
Addon tests were still starting to run before the node exp file
creation was complete.

- remove process_outputs_as_sources as it did not fix the
  problem
- update create_expfile.sh so that exp file is created in a
  temporary file and then renamed to final name so that
  file is only visible once it is complete
- update target used in building Addons so that for
  AIX it depends on the exp file being available

PR-URL: #8285
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

@mhdawson mhdawson deleted the mhdawson:aixaddon2 branch Mar 15, 2017

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