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

Revert "Fix relative paths in wasm backend source maps. Fixes #9837" #9924

Closed
wants to merge 1 commit into from

Conversation

@kripken
Copy link
Member

kripken commented Nov 29, 2019

Another option than reverting it is this possible fix https://github.com/emscripten-core/emscripten/compare/map2?expand=1 which I am waiting on tests for.

Based on the logs and reading the code, I think that should work, however I don't have a windows machine to test on. If you want to play it safe then I guess we can just revert.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 29, 2019

If you can verify the fix then I think we should land the fix yes. I we can't verify the fix on the windows machine i worry that we will up committing bunch of speculative fixes that might or might not pass on windows.

The cost of reverting and re-landing in pretty low so I'm tempted to revert. I also think its a good habit to get into too.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 29, 2019

That fix still looks odd to me: r'(?im)in main (|[/a-z\.\\]:).*test_lsan_leaks\.c:10:16$',

The second pair of brackets is supposed to match either a driver letter plus a colon or the start of a path (either abs or rel).

It seems to me like it should be more like (\.|\/|[a-z]:) to represents the three different ways to start a path?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 29, 2019

Maybe simpler just to do .*test_lsan_leaks.....?

@kripken
Copy link
Member

kripken commented Nov 29, 2019

Yeah, the square brackets could be adjusted and the regex cleaned up I guess. But meanwhile I realized the regex doesn't need to be changed at all - this should be just a 1-line fix.

I'd suggest landing that, but again, if you prefer to revert the original and delay everything til next week, I'm fine with that too.

I'm not sure what you're suggesting in the last comment?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 29, 2019

Happy to land you one line fix.

We should definitely cleanup the regex though. Right now its seem to be looking for driver letter a-z but also . and / as driver letters, which makes no sense.

@sbc100 sbc100 closed this Dec 5, 2019
@sbc100 sbc100 deleted the revert-9882-rel branch December 5, 2019 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants