8000 build: remove gyp build files by nornagon · Pull Request #14097 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

build: remove gyp build files #14097

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 9, 2018
Merged

build: remove gyp build files #14097

merged 9 commits into from
Sep 9, 2018

Conversation

nornagon
Copy link
Contributor
@nornagon nornagon commented Aug 14, 2018 •
edited by alexeykuzmin
Loading

a new era begins

Follow-up tasks:

notes: build system changed from GYP to GN

@nornagon nornagon requested a review from a team August 14, 2018 22:24
@MarshallOfSound
Copy link
Member

c4fvt

@codebytere
Copy link
Member

fb4

@ckerr
Copy link
Member
ckerr commented Aug 14, 2018

gasoline-is-cheap
8000

ckerr
ckerr approved these changes Aug 14, 2018
View reviewed changes
Copy link
Member
@ckerr ckerr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

🔥 ✨ 🔥 ✨ 🔥 ✨ 🔥

@nornagon nornagon requested a review from a team August 14, 2018 22:35
@ckerr
Copy link
Member
ckerr commented Aug 14, 2018

2fvsp2

@Kthulu120
Copy link
Contributor

Let it burn!!

@nornagon nornagon requested a review from a team August 15, 2018 03:57
@nornagon nornagon mentioned this pull request Aug 15, 2018
Merged
2 tasks
@alexeykuzmin alexeykuzmin self-assigned this Aug 29, 2018
@alexeykuzmin
Copy link
Contributor

@jkleinsc How can I remove gyp-based builds from AppVeyor?

@jkleinsc
Copy link
Member

@alexeykuzmin We can change the config in the Appveyor UI to ignore specific branches

@jkleinsc
Copy link
Member

@alexeykuzmin I updated the appveyor gyp builds to ignore this branch (delete-gyp) and once it is merged we can update to ignore master as well.

@alexeykuzmin alexeykuzmin requested a review from jkleinsc August 30, 2018 08:59
jkleinsc
jkleinsc requested changes Aug 30, 2018
View reviewed changes
Copy link
Member
@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Overall looks good to me - particularly all the GN builds including release are green! Just maybe some small tweaks which could be addressed after this is merged.

Also we need to update the release process to work with GN, but that can be done in separate PR.

@deepak1556
Copy link
Member

LGTM 👍 , just a couple of questions.

  • When is it planned to be merged ? The reason being if there are some build issues with gyp on master coming in Build error for missing pepper_flash.dll #14382 , just wanted to make sure if its worth fixing.

  • Should we plan for a blog post or a way to notify the community about this change before merging ?

@alexeykuzmin
Copy link
Contributor

@deepak1556 I want to merge this as soon as possible. It blocks the Ch67 upgrade.

Should we plan for a blog post or a way to notify the community about this change before merging ?

Sure. @ckerr maybe?

@deepak1556
Copy link
Member

I want to merge this as soon as possible. It blocks the Ch67 upgrade.

👍

Do you plan to address the TODO in the PR description about variable usage from electron.gyp in a follow up PR ?

@alexeykuzmin
Copy link
Contributor

Do you plan to address the TODO in the PR description about variable usage from electron.gyp in a follow up PR ?

@deepak1556
I would love to fix them before merge, but they can wait.

@alexeykuzmin alexeykuzmin changed the title [wip] build: remove gyp build files build: remove gyp build files Aug 30, 2018
@jkleinsc jkleinsc mentioned this pull request Sep 6, 2018
Closed
@alexeykuzmin
Copy link
Contributor
alexeykuzmin commented Sep 6, 2018 •
edited
Loading

@codebytere, @MarshallOfSound, @nitsakh, @zcbenz
I really want an explicit approval for this PR from you all as a signal that you're aware of the upcoming change: https://electronjs.org/blog/gn

If your approval is the last one, please feel free to click the green button =)

MarshallOfSound
MarshallOfSound approved these changes Sep 7, 2018
View reviewed changes
Copy link
Member
@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Some people just want to watch the world burn

nitsakh
nitsakh approved these changes Sep 7, 2018
View reviewed changes
Copy link
Contributor
@nitsakh nitsakh left a comment

Choose a reason for hiding this comment

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

🔥🚀

@zcbenz zcbenz merged commit 7252692 into master Sep 9, 2018
@release-clerk
Copy link
release-clerk bot commented Sep 9, 2018

Release Notes Persisted

build system changed from GYP to GN

@zcbenz zcbenz deleted the delete-gyp branch September 9, 2018 01:15
@ralphtheninja
Copy link

How can I track which version of node a particular version of electron is based on? Previously it was easy to just check vendor/node. Now I have no clue :)

@jkleinsc
Copy link
Member

@ralphtheninja https://github.com/electron/electron/blob/master/DEPS#L5 shows the sha for the commit on electron/node that is being used, for example:
electron/node@9dcbed2
Which points to Node 10.6.0.

Also, if you follow the GN build steps up to here: https://github.com/electron/electron/blob/master/docs/development/build-instructions-gn.md#getting-the-code
You can then find the checked out node directory at src/third_party/electron_node instead of the previous location vendor/node.

@ralphtheninja
Copy link

@jkleinsc Thank you!

@ckerr ckerr mentioned this pull request Dec 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@alexeykuzmin alexeykuzmin alexeykuzmin approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

@nitsakh nitsakh nitsakh approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

Assignees

@alexeykuzmin alexeykuzmin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0