Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 13, 2021

There are plans underway to start using pathlib more in emscripten.
See: #14074

This change a designed to test the water by using pathlib in test
code.

This allows us to use unix-like paths throughout the tests that will
get converted to windows-paths on windows machines automatically
by the pathlib library.

For example:

>>> str(pathlib.Path('foo/bar'))
'foo\\bar'

@sbc100 sbc100 requested review from aheejin and kripken May 13, 2021 15:35
@sbc100 sbc100 force-pushed the use_pathlib_in_tests branch 4 times, most recently from 6aa5010 to 69b3b96 Compare May 13, 2021 17:43
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

In general this looks good. We don't have a lot of windows testing here though, have you done any additional tests, or do you think this is enough?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 13, 2021

In general this looks good. We don't have a lot of windows testing here though, have you done any additional tests, or do you think this is enough?

Maybe I can do a CI run with windows running all of wasm2 and other? That should do it.

@kripken
Copy link
Member

kripken commented May 13, 2021

Sounds good. I guess the autoroller will also catch this eventually too.

@sbc100 sbc100 force-pushed the use_pathlib_in_tests branch 4 times, most recently from fd80061 to b914616 Compare May 14, 2021 00:40
@sbc100
Copy link
Collaborator Author

sbc100 commented May 14, 2021

All windows tests in other and wasm2 pass.. except for other.test_emsize for some reason, but that fails in github CI on windows both before and after this change so is unrealted.

@sbc100 sbc100 requested a review from kripken May 14, 2021 00:41
Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

This allows us to use unix-like paths throughout the tests that will
get converted to windows-paths on windows machines automatically
by the pathlib library.

Interesting, I wasn't aware this was possible. Thanks for working on this!

There are plans underway to start using pathlib more in emscripten.
See: #14074

This change a designed to test the water by using `pathlib` in test
code.

This allows us to use unix-like paths throughout the tests that will
get converted to windows-paths on windows machines automatically
by the pathlib library.
@sbc100 sbc100 force-pushed the use_pathlib_in_tests branch from b914616 to 0462788 Compare May 14, 2021 15:47
@sbc100 sbc100 enabled auto-merge (squash) May 14, 2021 16:37
@sbc100 sbc100 merged commit 13d6d6c into main May 14, 2021
@sbc100 sbc100 deleted the use_pathlib_in_tests branch May 14, 2021 16:39
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 14, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 15, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 15, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request May 15, 2021
Followup to #14175.  Again I think this makes many tests more readable.
sbc100 added a commit that referenced this pull request 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request 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 added a commit that referenced this pull request Aug 20, 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 added a commit that referenced this pull request Aug 20, 2021
…FC (#14916)

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
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.

4 participants