Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

build: automatically generate some file lists that the GN build uses #63

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

nornagon
Copy link
Member

This moves the hard-coded lists of source files into a generated .json file, which might make maintaining the GN build files a little easier, maybe?

@nornagon nornagon requested a review from codebytere September 19, 2018 01:27
@refack
Copy link
Contributor

refack commented Sep 19, 2018

Interesting idea. I was just considering coding a tool for going the other way from V8 gn files...
FYI: nodejs/node#22920, I'm refactoring severl .gyp files and lists. I'd be happy to port those here, when I'm done.

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Awesome idea. One thing though: is it not a priority to make the .py file PEP8 compliant?

@nornagon
Copy link
Member Author

@refack it would be super awesome if the "source of truth" for file lists like this was in a cross-build-system-compatible format, like JSON!

@ryzokuken good point, the python here was mostly POC, but I'll go clean it up :)

@nornagon
Copy link
Member Author

Updated to reformat and clean up the python a little.

BTW, I noticed most of the python files in tools/ aren't PEP-8 compatible either. Is that a standard that node recently adopted?

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

@nornagon 💯for the reformatting.

I don't even know if other peeps are node care a lot about PEP-8, but I think we should. I personally try to avoid straying away from it.

@nornagon nornagon merged commit eba2ae9 into electron-node-v10.10.0 Sep 21, 2018
@nornagon nornagon deleted the gn-build-automation branch September 21, 2018 00:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants