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.

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

[v12.x backport] module: drop support for extensionless main entry points in esm #32280

Closed
wants to merge 1 commit into from
Closed

[v12.x backport] module: drop support for extensionless main entry points in esm #32280

wants to merge 1 commit into from

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Mar 15, 2020 •
edited
Loading

Backport of #31415. Thank you @codebytere for the prompt!

This isn't as clean as it could be because of 04d07ed. @bmeck or @guybedford do you mind double-checking this? I'm assuming that we don't want to merge into 12 the commit that was reverted.

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. v12.x labels Mar 15, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@puzpuzpuz puzpuzpuz mentioned this pull request Mar 15, 2020
Closed
4 tasks
@puzpuzpuz
Copy link
Member

This PR doesn't edit repl.md. Is v12.x-staging in a broken state?

I've created #32282 to deal with that.

@codebytere

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@codebytere

This comment has been minimized.

Sign in to view
@guybedford guybedford force-pushed the backport-31415-to-12.x-staging branch from bb4fbbe to 3e13dc7 Compare March 23, 2020 01:34
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Mar 24, 2020 •
edited
Loading

@codebytere I've been struggling with this for over a week now and I just can't get CI to pass.

From what I can tell of the CI jobs that are failing, like node-test-commit-aix, it appears that make test-ci and/or make run-ci are failing:

18:11:37 gmake[1]: *** [Makefile:529: test-ci] Error 1
18:11:37 gmake: *** [Makefile:557: run-ci] Error 2

When I run locally, I get this error with make run-ci:

not ok 636 parallel/test-fs-stat-bigint
  ---
  duration_ms: 0.248
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:385
        throw err;
        ^

    AssertionError [ERR_ASSERTION]: Number version atimeMs = 1585172518639.4456, BigInt version atimeMs = 1585172518625n, Allowable delta = 14
        at verifyStats (/Users/geoffrey/Sites/node/test/parallel/test-fs-stat-bigint.js:71:7)

Perhaps we need to increase the allowable delta here?

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth GeoffreyBooth mentioned this pull request Mar 25, 2020
Closed
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth
Copy link
Member Author

@codebytere I keep resuming CI, but node-test-commit-aix keeps failing. It fails for v12.x-staging too: https://ci.nodejs.org/job/node-test-commit/36866/.

Can we land this, since the problem appears to be in the staging branch?

@MylesBorins
Copy link
Contributor

As the failure seems unrelated to this PR and on the staging branch I think this is ok to land

@codebytere thoughts?

MylesBorins
MylesBorins approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
@codebytere
Copy link
Member

codebytere commented Apr 1, 2020 •
edited
Loading

@GeoffreyBooth staging should be good to rebase against, then we can get this landed.

@GeoffreyBooth @guybedford
module: drop support for extensionless main entry points in esm
b6ad757 
PR-URL: nodejs#31415
Backport-PR-URL: nodejs#32280
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@guybedford guybedford force-pushed the backport-31415-to-12.x-staging branch from 0da01a7 to b6ad757 Compare April 1, 2020 02:36
@guybedford
Copy link
Contributor

I've updated the rebase here.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020 •
edited by MylesBorins
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30314/
rerun linuxone: https://ci.nodejs.org/job/node-test-commit-linuxone/19987/

MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@GeoffreyBooth @MylesBorins
module: drop support for extensionless main entry points in esm
4ee41c5 
Backport-PR-URL: #32280
PR-URL: #31415
Backport-PR-URL: #32280
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

finally green!

Landed in 4ee41c5

@MylesBorins MylesBorins closed this Apr 1, 2020
@GeoffreyBooth GeoffreyBooth deleted the backport-31415-to-12.x-staging branch April 1, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awibox awibox awibox left review comments

@MylesBorins MylesBorins MylesBorins approved these changes

@bmeck bmeck bmeck approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@GeoffreyBooth @nodejs-github-bot @puzpuzpuz @codebytere @MylesBorins @guybedford @bmeck @awibox