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

[v14.x backport] errors: do not call resolve on URLs with schemes #37717

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Mar 11, 2021

We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: #35325

PR-URL: #35903
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v14.x labels Mar 11, 2021
@bcoe bcoe changed the title errors: do not call resolve on URLs with schemes [v14.x backport] errors: do not call resolve on URLs with schemes Mar 11, 2021
@bcoe bcoe requested review from ruyadorno, aduh95 and Trott March 11, 2021 19:30
bcoe added a commit to bcoe/node-1 that referenced this pull request Mar 11, 2021
We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: nodejs#35325

Backport-PR-URL: nodejs#37717
PR-URL: nodejs#35903
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bcoe bcoe force-pushed the backport-webpack-fix branch from 8cae83d to 1cde0f9 Compare March 11, 2021 19:31
@bcoe
Copy link
Contributor Author

bcoe commented Mar 11, 2021

This is part of a release train of backport PRs:

#33491
#35903 🚆
#35915
#37252
#37362

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 12, 2021

@bcoe bcoe force-pushed the backport-webpack-fix branch from 35bd63e to 1cde0f9 Compare March 15, 2021 18:26
@bcoe bcoe force-pushed the backport-webpack-fix branch from 1cde0f9 to 2197404 Compare March 16, 2021 00:47
bcoe added a commit to bcoe/node-1 that referenced this pull request Mar 16, 2021
We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: nodejs#35325

Backport-PR-URL: nodejs#37717
PR-URL: nodejs#35903
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bcoe bcoe force-pushed the backport-webpack-fix branch from 2197404 to f91f140 Compare March 16, 2021 00:59
@bcoe
Copy link
Contributor Author

bcoe commented Mar 16, 2021

@aduh95 any idea where to go next with the test-asan issue. When I run locally --enable-asan fails on completely different tests (I'm guessing related to running on OSX, not this same failure we're seeing).

@aduh95
Copy link
Contributor

aduh95 commented Mar 16, 2021

@aduh95 any idea where to go next with the test-asan issue. When I run locally --enable-asan fails on completely different tests (I'm guessing related to running on OSX, not this same failure we're seeing).

Sorry I can't explain the failure either, it's really weird.

@ruyadorno
Copy link
Member

@bcoe is this same target failing the same way if you try running them in v14.x-staging?

@bcoe
Copy link
Contributor Author

bcoe commented Mar 16, 2021

@ruyadorno v14.x-staging seems fine, so something about this PR is breaking the test. The weird thing is the test isn't failing on test-asan on the main branch either (which includes this change).

We might need to try to reproduce in a container locally.

@bcoe bcoe force-pushed the backport-webpack-fix branch 2 times, most recently from 5c95073 to 0e1a9b5 Compare March 24, 2021 04:23
bcoe added a commit to bcoe/node-1 that referenced this pull request Apr 4, 2021
We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: nodejs#35325

Backport-PR-URL: nodejs#37717
PR-URL: nodejs#35903
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bcoe bcoe force-pushed the backport-webpack-fix branch from e59434d to e4ad78b Compare April 4, 2021 01:00
@bcoe
Copy link
Contributor Author

bcoe commented Apr 5, 2021

@targos @richardlau I've been unable to reproduce the test-asan issues on my own linux machine, and they seem to be happening in tests unrelated to this PR.

I noticed in #38001 we also landed a PR to 14.x-staging with test-asan failures, is it alright if I do likewise with this PR? (perhaps with a tracking issue to dig into test-asan issues on 14.x-staging).

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: nodejs#35325

PR-URL: nodejs#35903
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos force-pushed the backport-webpack-fix branch from e4ad78b to 753d783 Compare April 24, 2021 12:53
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

targos pushed a commit that referenced this pull request Apr 25, 2021
We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: #35325

PR-URL: #35903
Backport-PR-URL: #37717
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos
Copy link
Member

targos commented Apr 25, 2021

Landed in b01c496

@targos targos closed this Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants