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: update gyp to 0.5.0 #32698

Closed
wants to merge 5 commits into from
Closed

Conversation

ryzokuken
Copy link
Contributor

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

🎉

/cc @targos @cclauss @nodejs/gyp

Thanks for all the lovely work so far, everyone ❤️

@ryzokuken ryzokuken requested review from targos and cclauss April 7, 2020 01:39
@ryzokuken ryzokuken self-assigned this Apr 7, 2020
@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 Apr 7, 2020
@aduh95
Copy link
Contributor

aduh95 commented Apr 7, 2020

Is there a release note for gyp 0.2.0 available somewhere?

@ryzokuken
Copy link
Contributor Author

@aduh95 we're working on adding a changelog to gyp-extra in nodejs/gyp-next#27. Also, it's hard to add notes for v0.2.0 since it's the first real release of gyp-next and a large percentage of development was done inside this tree, so the history is fragmented. That said, it'll be much better moving forwards, I'd say.

@mmarchini
Copy link
Contributor

Nice work! @ryzokuken since a full changelog is probably unfeasible, are there any notable changes you'd like to share?

@targos
Copy link
Member

targos commented Apr 7, 2020

The only changes between the current code in node-gyp are related to CI, governance and code style (linter).
This should be functionally equivalent.

@gengjiawen
Copy link
Member

windows failed with:

[vcvarsall.bat] Environment initialized for: 'x64'
Found MSVS version 16.0
configure  --dest-cpu=x64
Node.js configure: Found Python 3.8.2...
Traceback (most recent call last):
  File "configure", line 24, in <module>
    import configure
  File "D:\a\node\node\configure.py", line 1811, in <module>
    run_gyp(gyp_args)
  File "tools\gyp_node.py", line 54, in run_gyp
    rc = gyp.main(args)
  File "tools\gyp\pylib\gyp\__init__.py", line 679, in main
    return gyp_main(args)
  File "tools\gyp\pylib\gyp\__init__.py", line 664, in gyp_main
    generator.GenerateOutput(flat_list, targets, data, params)
  File "tools\gyp\pylib\gyp\generator\msvs.py", line 2196, in GenerateOutput
    sln = MSVSNew.MSVSSolution(
  File "tools\gyp\pylib\gyp\MSVSNew.py", line 231, in __init__
    self.Write()
  File "tools\gyp\pylib\gyp\MSVSNew.py", line 259, in Write
    f.write(
  File "tools\gyp\pylib\gyp\common.py", line 419, in write
    self.tmp_file.write(s.encode("utf-8"))
TypeError: write() argument must be str, not byte

prefix=os.path.split(filename)[1] + '.gyp.',
dir=base_temp_dir)
try:
self.tmp_file = os.fdopen(tmp_fd, 'wb')
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are opening in wb mode...

dir=base_temp_dir,
)
try:
self.tmp_file = os.fdopen(tmp_fd, "w")
Copy link
Contributor

Choose a reason for hiding this comment

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

... and here we are opening in w mode.

Copy link
Member

Choose a reason for hiding this comment

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

Is it wrong?
It came from this upstream commit that was ported in nodejs/gyp-next#11

Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure but it would explain the TypeError: write() argument must be str, not byte above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cclauss @targos @gengjiawen could someone point me to the smallest configuration that causes gyp to fail? We could simply add a failing test and patch the relevant line highlighted by @cclauss.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like gyp-next has not been add windows to CI, we can make this as a start ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gengjiawen indeed, it has been blocked on the test failures in nodejs/gyp-next#8. I'll try to get it resolved ASAP.

@BridgeAR
Copy link
Member

@ryzokuken this needs a rebase and is this otherwise ready to review again?

@ryzokuken
Copy link
Contributor Author

@BridgeAR yeah, I need to make a couple of changes. Sorry for being tardy, I'll find some time to do it later this week. Thanks.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@targos
Copy link
Member

targos commented Jun 15, 2020

@ryzokuken would you like to update this to v0.3.0? Otherwise I can do it.

@ryzokuken
Copy link
Contributor Author

@targos sure.

@ryzokuken ryzokuken changed the title tools: update gyp to 0.2.0 tools: update gyp to 0.4.0 Jul 15, 2020
@ryzokuken
Copy link
Contributor Author

@targos PTAL.

@targos
Copy link
Member

targos commented Jul 15, 2020

there's a conflict

@gengjiawen gengjiawen requested a review from cclauss July 15, 2020 06:54
@ryzokuken
Copy link
Contributor Author

ugh, why did someone push code to a dependency? Let me take a look :/

@gengjiawen
Copy link
Member

ugh, why did someone push code to a dependency? Let me take a look :/

Most likely #32867.

@ryzokuken
Copy link
Contributor Author

Most likely #32867.

This is definitely it. We need to work harder to make sure all changes to gyp land in the upsteam repo first 😅

@targos do you think moving gyp to deps/gyp would help solve this issue?

@ryzokuken
Copy link
Contributor Author

also, I fixed the merge conflicts locally, but I'm not sure how to proceed with this. Should I upstream these changes to gyp-next, make a new release and update this PR?

@gengjiawen
Copy link
Member

also, I fixed the merge conflicts locally, but I'm not sure how to proceed with this. Should I upstream these changes to gyp-next, make a new release and update this PR?

+1 on make a new release.

@nodejs-github-bot
Copy link
Collaborator

@ryzokuken
Copy link
Contributor Author

CI passed! Landing this.

ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Oct 4, 2020

Landed in b79829c...f215a4d 🎉

@ryzokuken ryzokuken closed this Oct 4, 2020
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants