GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node: Update strings node.js -> bootstrap_node.js by joshgav · Pull Request #5962 · nodejs/node

Conversation

@joshgav

@joshgav joshgav commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

node.cc

Description of change

Update filename in node.cc and tests to bootstrap_node.js per #5103 and update nearby comments. Fixed some other comments while I was at it.

Will need to rebase once #5881 lands.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@evanlucas

evanlucas commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

/cc @Fishrock123
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Mar 30, 2016
Fishrock123
Fishrock123 reviewed Mar 30, 2016
View reviewed changes
Comment thread test/message/core_line_numbers.out Outdated

@Fishrock123 Fishrock123 Mar 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

curious that the message tests pass without this change?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Mar 30, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Guessing it was ExecuteString() setting the script name to node.js.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joshgav joshgav Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, ExecuteString calls v8::Script::Compile which allows us to specify the ScriptOrigin, which comes from the FIXED_ONE_BYTE_STRING I changed. grep ExecuteString ./src/* -r says it isn't called anywhere else, perhaps ExecuteString should be merged into LoadEnvironment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

These commits need to be separate PRs.
joshgav reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 30, 2016
@Fishrock123

Fishrock123 commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

I'm also pretty sure this is a breaking change, cc @nodejs/ctc

Is this 100% necessary, even if previously it was now not technically correct?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

@joshgav was the set_trace_sync_io part causing any problems anywhere? If so, can you provide a test case?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joshgav
node: fix comments and strings
ddf7a78 
Clarify comments re invoking bootstrap_node.js.
Fix filename to bootstrap_node.js per nodejs#5103.
Fix tests `node.js` -> `bootstrap_node.js`
Fix comment on why we check the loop again before exiting.
`context-inl.h` -> `env-inl.h`
@joshgav
joshgav force-pushed the joshgav-dev branch from ec69017 to ddf7a78 Compare March 30, 2016 22:55
@joshgav joshgav changed the title node: Update strings, fix --trace-sync-io. node: Update strings node.js -> bootstrap_node.js Mar 30, 2016
@joshgav

joshgav commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

moved --trace-sync-io fix to its own PR #5964, will provide a test there
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joshgav

joshgav commented Mar 30, 2016

Copy link
Copy Markdown
Contributor Author

@Fishrock123 would internal/bootstrap_node.js be better than just bootstrap_node.js? It seems to have the same effect, might as well get it right now. IMO the reason to fix this is to help people when debugging, so the internal prefix would probably be best.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

cc @nodejs/ctc

I think leaving it alone would probably be o.k., but if it should be changed that's fine.

It shouldn't contain the path though, no internal/, it's just representative of the filename. Any file search will pull it out pretty easily. (searching for node.js shows boostrap_node.js in my editor' file finder)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

Is the question whether or not internal/ should appear in the stack traces? If so, I think it should be in there.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joshgav

joshgav commented Mar 31, 2016

Copy link
Copy Markdown
Contributor Author

The question is two-fold:

  1. Do we leave the traced script name as is - i.e. node.js - and somehow document that the actual script is at ./lib/internal/bootstrap_node.js.
  2. If we change the traced script name, do we use internal/bootstrap_node.js or bootstrap_node.js.

Personally I prefer including internal/ cause it helps with following the traces.

@Fishrock123 Do we have the option of perhaps renaming as internal/node.js (i.e. remove bootstrap_)? That might make option 1 above more palatable.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

IMO we should update it to most accurately reflect reality if possible.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

Is this only exposed to error logs? I am relatively concerned we might be breaking something because I'm not entirely sure what this changes.

CI: https://ci.nodejs.org/job/node-test-pull-request/2149/

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/177/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@estliberitas
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@joshgav joshgav closed this May 9, 2016
@joshgav
joshgav deleted the joshgav-dev branch May 9, 2016 17:51
@joshgav

joshgav commented May 9, 2016

Copy link
Copy Markdown
Contributor Author

I moved this fix to another branch in my fork; if we want to continue the discussion let me know and I'll open a corresponding new PR. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented May 9, 2016

Copy link
Copy Markdown
Contributor

Oh shoot

I guess? we should do this but I'm not the best person to ask about that
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@joshgav @evanlucas @Fishrock123 @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.