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

Fix test-wpt and test-css for Windows. #14039

Merged
merged 1 commit into from Nov 16, 2016

Conversation

@metajack
Copy link
Contributor

commented Nov 3, 2016

In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because testing the tests is too meta for me

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Nov 3, 2016

Heads up! This PR modifies the following files:

@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

r? @larsbergstrom

Also, perhaps @jgraham should look at the openssl change.

@highfive highfive assigned larsbergstrom and unassigned pcwalton Nov 3, 2016
@jgraham

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

Remeber the wptrunner changes need to be upstreamed.


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


python/mach_bootstrap.py, line 79 at r1 (raw file):

# Possible names of executables
if sys.platform in ['msys', 'win32']:

This could use a comment about why.


tests/wpt/web-platform-tests/tools/sslutils/openssl.py, line 60 at r1 (raw file):

        self.cmd += list(args)

        env = {}

Why?


Comments from Reviewable

@metajack metajack force-pushed the metajack:windows-wpt branch from 72333c1 to bce8980 Nov 3, 2016
@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

@jgraham Added comments as requested. I'll prepare an upstream PR with the same diff in a bit.

@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Upstream PR is w3c/wpt-tools#139

@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Upstream PR has landed.

@jgraham

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@metajack metajack force-pushed the metajack:windows-wpt branch from bce8980 to 77ccb15 Nov 3, 2016
@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Updated to add correct PATH for AppVeyor gnu builds.

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

That has successfully returned us to the status quo of #13939.

@jgraham

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

📌 Commit 77ccb15 has been approved by jgraham

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

⌛️ Testing commit 77ccb15 with merge 92115c7...

bors-servo added a commit that referenced this pull request Nov 4, 2016
Fix test-wpt and test-css for Windows.

<!-- Please describe your changes on the following line: -->
In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because testing the tests is too meta for me

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14039)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

💔 Test failed - windows-dev

@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

@bors-servo retry

  • had to update windows builder for the correct PATH
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

⌛️ Testing commit 77ccb15 with merge 9c2bb06...

bors-servo added a commit that referenced this pull request Nov 4, 2016
Fix test-wpt and test-css for Windows.

<!-- Please describe your changes on the following line: -->
In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because testing the tests is too meta for me

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14039)
<!-- Reviewable:end -->
@highfive highfive removed the S-tests-failed label Nov 4, 2016
@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

⌛️ Trying commit 7f8412c with merge 87f0d2c...

bors-servo added a commit that referenced this pull request Nov 16, 2016
Fix test-wpt and test-css for Windows.

<!-- Please describe your changes on the following line: -->
In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because testing the tests is too meta for me

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14039)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

💔 Test failed - windows-dev

@metajack metajack force-pushed the metajack:windows-wpt branch from 7f8412c to 77ccb15 Nov 16, 2016
In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).
@metajack metajack force-pushed the metajack:windows-wpt branch from 77ccb15 to 8e8519d Nov 16, 2016
@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2016

@bors-servo retry

  • another time into the breach
@metajack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2016

@bors-servo r=jgraham

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

📌 Commit 8e8519d has been approved by jgraham

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

⌛️ Testing commit 8e8519d with merge 17bf6ae...

bors-servo added a commit that referenced this pull request Nov 16, 2016
Fix test-wpt and test-css for Windows.

<!-- Please describe your changes on the following line: -->
In addition to minor changes for Windows, this forces Windows Python to
be used for all Windows builds (instead of using Windows Python only for
pc-windows-msvc builds).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because testing the tests is too meta for me

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14039)
<!-- Reviewable:end -->
@bors-servo bors-servo referenced this pull request Nov 16, 2016
3 of 3 tasks complete
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

@bors-servo bors-servo merged commit 8e8519d into servo:master Nov 16, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@metajack metajack deleted the metajack:windows-wpt branch Nov 16, 2016
@upsuper

This comment has been minimized.

Copy link
Member

commented on python/servo/command_base.py in 8e8519d Nov 21, 2016

This change breaks building geckolib inside MozillaBuild. MozillaBuild is basically just a MINGW32 environment with Windows Python, so it has MSYSTEM environment.

This comment has been minimized.

Copy link
Contributor Author

replied Nov 21, 2016

I suggest adding a --target flag to mach then. Building from MSYSTEM seems like it should build mingw by default, at least until we make msvc the only target.

This comment has been minimized.

Copy link
Member

replied Nov 21, 2016

Sounds reasonable.

This comment has been minimized.

Copy link
Member

replied Nov 21, 2016

Actually, can we just use native rustc rather than downloading rustc for building geckolib?

This comment has been minimized.

Copy link
Contributor Author

replied Nov 21, 2016

You can override how rustc is chosen by using .servobuild. There should be an example in the file.

@nox

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

This patch makes people with non-UTF-8 environment variables unable to run tests. Was that intended? Can that be reverted?

@Manishearth

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

Can someone address @nox's question? This is affecting me too.

@larsbergstrom

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

The problem according to the comment is that non-UTF-8 environment variables cannot be passed to create process. What should we be doing here - it sounds like a fail either way?

I assume the options are:

  1. Only do the UTF-8 filtering on windows
  2. Just drop non-UTF-8 environment variables
  3. Revert the patch, rebreak windows
    ?
@Manishearth

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

I looked into this more, test-wpt now actually handles the error. test-css however has an older copy of sslutils.py which doesn't deal with this.

#15452 (comment)

I'm not sure which of these files we can edit and what the canonical source is.

@jdm

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

You can do anything you want to test-css, because it's going away in #19070. The canonical source for those files is upstream web-platform-tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.