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: runtime-deprecate requiring deps #16392

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Fixes: #15566 (comment)

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
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. labels Oct 22, 2017
@TimothyGu TimothyGu mentioned this pull request Oct 22, 2017
4 tasks
@TimothyGu TimothyGu requested a review from bnoordhuis October 22, 2017 22:02
@refack
Copy link
Contributor

refack commented Oct 22, 2017

Isn't this major

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

FWIW, I'd be okay with simple removal. It seems vanishingly unlikely that anyone is using these.

tools/js2c.py Outdated
@@ -256,10 +264,19 @@ def JS2C(source, target):
lines = ExpandConstants(lines, consts)
lines = ExpandMacros(lines, macros)

deprecatedDeps = None
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: deprecated_deps

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

tools/js2c.py Outdated
if split[0] == 'deps':
if split[1] == 'node-inspect' or split[1] == 'v8':
deprecatedDeps = split[1:]
split = ['internal'] + split
Copy link
Member

Choose a reason for hiding this comment

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

This maps deps/foo/bar.js to internal/deps/foo/bar.js.

I don't think you have to special-case for node-inspect or v8 because anything from deps/ should pretty much be internal by default. ISTM it can be simplified to his:

if split[0] === 'deps':
  split[0] = 'internal'
else:
  split = split[1:]

Could be simplified further by just matching on name:

name = '/'.join(re.split('/|\\\\', name))
if name.startswith('lib/'):
  name = name[len('lib/'):]
elif name.startswith('deps/'):
  deprecated_deps, name = name, 'internal/' + name[len('deps/'):]

Bonus points: you won't have to '/'.join(...) below.

Copy link
Member

Choose a reason for hiding this comment

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

I think I misread the intent of this code - it's prefixing with internal/ unconditionally but stashing the original away. Could still be simplified, though:

name = '/'.join(re.split('/|\\\\', name))
if name.startswith('deps/node-inspect/') or name.startswith('deps/v8/'):
  deprecated_deps = name[len('deps/'):]
  name = 'internal/' + name
elif name.startswith('lib/'):
  name = name[len('lib/'):]

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Not only that, but internal/ should also be added to any other file in deps/ that do not match node-inspect and v8 to prevent them from being visible to userland, thus the code in its current form.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM once @bnoordhuis is happy also.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM once nits are addressed.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

generally LGTM once @bnoordhuis is happy also

@TimothyGu
Copy link
Member Author

@TimothyGu
Copy link
Member Author

Landed in 0e10717.

@TimothyGu TimothyGu closed this Nov 13, 2017
@TimothyGu TimothyGu deleted the deps-modules branch November 13, 2017 18:51
TimothyGu added a commit that referenced this pull request Nov 13, 2017
PR-URL: #16392
Fixes: #15566 (comment)
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request Nov 29, 2017
3 tasks
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. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants