Skip to content

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 ❤️

@ryzokukenryzokuken requested review from cclauss and targosApril 7, 2020 01:39
@ryzokukenryzokuken self-assigned this Apr 7, 2020
@nodejs-github-botnodejs-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

Is there a release note for gyp 0.2.0 available somewhere?

@ryzokuken
Copy link
ContributorAuthor

@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

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.0configure --dest-cpu=x64Node.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

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

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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

@BridgeARBridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
@targos
Copy link
Member

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

@ryzokuken
Copy link
ContributorAuthor

@targos sure.

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

@targos PTAL.

@targos
Copy link
Member

there's a conflict

@gengjiawengengjiawen requested a review from cclaussJuly 15, 2020 06:54
@ryzokuken
Copy link
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

CI passed! Landing this.

ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
ryzokuken added a commit that referenced this pull request Oct 4, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@ryzokuken
Copy link
ContributorAuthor

ryzokuken commented Oct 4, 2020

Landed in b79829c...f215a4d 🎉

danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@danielleadamsdanielleadams 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 <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Aug 18, 2021
PR-URL: nodejs#32698 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ryzokuken@aduh95@mmarchini@targos@gengjiawen@BridgeAR@cclauss@nodejs-github-bot@richardlau