Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 19, 2021

This change is very much like that large change I made to the test
suite. See #14191 and #14175.

It enables is to use unix style paths rather than arrays of path
components which has several advantages. As well as beeing more
readable and writable it also allows for things like git grep "musl/src" to work as expected. For an example of how much more
readable it makes the code see tools/system_libs.py.

This change was made almost exclusively with a bunch of sed
commands.

Because there is likely some minor performance overhead I did
some measurements.

Running ./tests/runner wasm2 --skip-slow:

before:
real 1m10.403s
user 49m40.135s
sys 6m14.466s
after:
real 1m10.397s
user 49m43.849s
sys 6m10.698s

Running ./embuilder build libc --force:

before:
real 0m5.597s
user 3m2.721s
sys 0m50.843s
after:
real 0m5.609s
user 3m0.998s
sys 0m49.796s

@sbc100 sbc100 force-pushed the path_from_root branch 3 times, most recently from 2588c19 to 6da0316 Compare August 19, 2021 23:24
@sbc100 sbc100 requested review from dschuff and kripken August 19, 2021 23:24
@dschuff
Copy link
Member

dschuff commented Aug 19, 2021

I'm not familiar with pathlib, so I checked it out.
It looks like it doesn't have any abstraction for mac paths (they are just PosixPath), which is fine except that IIRC Mac filesystems are case-insensitive by default (whereas PosixPath treats them as case-sensitive). So, probably not a big problem if we already work OK on both Linux and Windows, but maybe something to watch out for.

@dschuff
Copy link
Member

dschuff commented Aug 19, 2021

This is pretty nice and readable though.
as a side note, what would be the downside of just always using forward slash on windows, even without pathlib to canonicalize it? AFAIK, forward slashes just work fine on windows, and when I do random scripts or throwaway code I always just use / even on Windows

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 20, 2021

This is pretty nice and readable though.
as a side note, what would be the downside of just always using forward slash on windows, even without pathlib to canonicalize it? AFAIK, forward slashes just work fine on windows, and when I do random scripts or throwaway code I always just use / even on Windows

I don't know what the major downsides would be (bit I imagine there must be some?), but I like the pathlib is consistent with what we do today (os.path.join). Keeps this change NFC for sure.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 20, 2021

I guess its nice for windows users when they see pathnames in log files or error message, or otherwise echo'd back at them if they look like windows paths.

This change is very much like that large change I made to the test
suite. See #14191 and #14175.

It enables is to use unix style paths rather than arrays of path
components which has several advantages.  As well as beeing more
readable and writable it also allows for things like `git grep
"musl/src"` to work as expected.  For an example of how much more
readable it makes the code see `tools/system_libs.py`.

This change was made almost exclusively with a bunch of `sed`
commands.

Because there is likely some minor performance overhead I did
some measurements.

Running `./tests/runner wasm2 --skip-slow`:

before:
real    1m10.403s
user    49m40.135s
sys     6m14.466s
after:
real    1m10.397s
user    49m43.849s
sys     6m10.698s

Running `./embuilder build libc --force`:

before:
real    0m5.597s
user    3m2.721s
sys     0m50.843s
after:
real    0m5.609s
user    3m0.998s
sys     0m49.796s
@sbc100 sbc100 merged commit d8ec08a into main Aug 20, 2021
@sbc100 sbc100 deleted the path_from_root branch August 20, 2021 16:42
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.

3 participants