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

build: automatically generate list of library_files #3506

Closed
wants to merge 2 commits into from
Closed

build: automatically generate list of library_files #3506

wants to merge 2 commits into from

Conversation

chrisdickinson
Copy link
Contributor

Automatically generate the list of library_files by using Python's os.walk so we don't have to remember to specify new files (or remember to remove old ones.)

Example output:

# Do not edit. Generated by the configure script.
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'asan': 0,
                 'host_arch': 'x64',
                 'icu_small': 'false',
                 'library_files': [ 'src/node.js',
                                    'lib/_debug_agent.js',
                                    'lib/_debugger.js',
                                    'lib/_http_agent.js',
                                    'lib/_http_client.js',
                                    'lib/_http_common.js',
                                    'lib/_http_incoming.js',
                                    'lib/_http_outgoing.js',
                                    'lib/_http_server.js',
                                    'lib/_linklist.js',
                                    'lib/_stream_duplex.js',
                                    'lib/_stream_passthrough.js',
                                    'lib/_stream_readable.js',
                                    'lib/_stream_transform.js',
                                    'lib/_stream_wrap.js',
                                    'lib/_stream_writable.js',
                                    'lib/_tls_common.js',
                                    'lib/_tls_legacy.js',
                                    'lib/_tls_wrap.js',
                                    'lib/assert.js',
                                    'lib/buffer.js',
                                    'lib/child_process.js',
                                    'lib/cluster.js',
                                    'lib/console.js',
                                    'lib/constants.js',
                                    'lib/crypto.js',
                                    'lib/dgram.js',
                                    'lib/dns.js',
                                    'lib/domain.js',
                                    'lib/events.js',
                                    'lib/freelist.js',
                                    'lib/fs.js',
                                    'lib/http.js',
                                    'lib/https.js',
                                    'lib/module.js',
                                    'lib/net.js',
                                    'lib/os.js',
                                    'lib/path.js',
                                    'lib/process.js',
                                    'lib/punycode.js',
                                    'lib/querystring.js',
                                    'lib/readline.js',
                                    'lib/repl.js',
                                    'lib/stream.js',
                                    'lib/string_decoder.js',
                                    'lib/sys.js',
                                    'lib/timers.js',
                                    'lib/tls.js',
                                    'lib/tty.js',
                                    'lib/url.js',
                                    'lib/util.js',
                                    'lib/v8.js',
                                    'lib/vm.js',
                                    'lib/zlib.js',
                                    'lib/internal/child_process.js',
                                    'lib/internal/freelist.js',
                                    'lib/internal/linkedlist.js',
                                    'lib/internal/module.js',
                                    'lib/internal/readme.md',
                                    'lib/internal/repl.js',
                                    'lib/internal/socket_list.js',
                                    'lib/internal/util.js',
                                    'lib/internal/streams/lazy_transform.js'],
                 'llvm_version': 0,
                 'node_byteorder': 'little',
                 'node_install_npm': 'true',
                 'node_prefix': '/usr/local',
                 'node_release_urlbase': '',
                 'node_shared_http_parser': 'false',
                 'node_shared_libuv': 'false',
                 'node_shared_openssl': 'false',
                 'node_shared_zlib': 'false',
                 'node_tag': '',
                 'node_use_dtrace': 'true',
                 'node_use_etw': 'false',
                 'node_use_lttng': 'false',
                 'node_use_openssl': 'true',
                 'node_use_perfctr': 'false',
                 'openssl_fips': '',
                 'openssl_no_asm': 0,
                 'python': '/usr/local/opt/python/bin/python2.7',
                 'target_arch': 'x64',
                 'uv_parent_path': '/deps/uv/',
                 'uv_use_dtrace': 'true',
                 'v8_enable_gdbjit': 0,
                 'v8_enable_i18n_support': 0,
                 'v8_no_strict_aliasing': 1,
                 'v8_optimized_debug': 0,
                 'v8_random_seed': 0,
                 'v8_use_snapshot': 1,
                 'want_separate_host_toolset': 0}}

@chrisdickinson chrisdickinson added the build Issues and PRs related to build files or the CI. label Oct 24, 2015
o['variables']['library_files'] = modules
lib_dir = os.path.join(root_dir, 'lib')
for root, dirs, files in os.walk(lib_dir):
modules += map(lambda xs: os.path.join(root, xs)[2:], files)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let us say that my editor creates a temporary file, then if I run ./configure, it will include that as well. Probably, we need to filter out the files which end with .js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be addressed now. Good catch!

@bnoordhuis
Copy link
Member

so we don't have to remember to specify new files (or remember to remove old ones.)

I don't really see the point. If you forget to add a new file, you'll find out soon enough because tests will start failing. Similarly, when you forget to remove a deleted file from the list, the build will fail at the j2sc step.

@chrisdickinson
Copy link
Contributor Author

If you forget to add a new file, you'll find out soon enough because tests will start failing. Similarly, when you forget to remove a deleted file from the list, the build will fail at the j2sc step.

I'm mostly scratching an itch, here — the extra step of making sure the file was present in the gypfile is easy to forget (until you recompile and test), wasn't obvious when I was a newcomer, and presents a human-monitored style rule in PRs (I recall there was a comment about putting the files in the correct alphabetical order in the gypfile at some point?) It's definitely not a huge change, but I've run into it enough that I wanted to smooth this corner.

@jbergstroem
Copy link
Member

I'm -0 to this PR but there's a pretty convincing argument regarding where to look/edit if you're actually adding a file to lib/. Can we improve feedback somehow instead?

@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

LGTM, but I'm +0.1 on whether it's needed ;-)

@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

heh, +0.1 for me too, that gets us to +0.2, lgtm

@Fishrock123
Copy link
Contributor

Uh, what's the reason to not do this programatically? SGTM? (I don't want to sign off because python tho)

@Trott
Copy link
Member

Trott commented Feb 28, 2016

@chrisdickinson Is this something you still want? Or would you be inclined to close this? (I have no opinion either way, just trying to tidy up inactive PRs a bit.)

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

ping @chrisdickinson

@jbergstroem
Copy link
Member

@Fishrock123 the counter argument would be that we're introducing an additional layer which down the road could be more error prone than just editing the file. Another (far fetched) argument would be that configure is not meant to be a preprocessor -- that's gyp's job.

@Fishrock123
Copy link
Contributor

Meh. The build will just fail if you don't edit it anyways. let's close unless someone wants to pick it up.

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. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants