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

Port some code to Python3 #27999

Closed
wants to merge 1 commit into from
Closed

Port some code to Python3 #27999

wants to merge 1 commit into from

Conversation

ghostd
Copy link
Contributor

@ghostd ghostd commented Dec 28, 2020

With this PR i wish to share modifications to run mach with ./mach instead of python3 ./mach.

I'm using a Debian machine. The following commands work for me:

  • ./mach build --dev
  • ./mach test-tidy
  • ./mach fmt

I still have errors with the WPT scripts, but maybe the root causes is a bad resolution of requirements.txt

I used 2to3 to migrate some code before applying some manual fixes. For PLY, I copied/paste the code from the github repository.

If someone can test...
For now i'm not sure how to continue the migration while i can't run WPT :-/


  • ./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 ___

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/codegen/ply/README, components/script/dom/bindings/codegen/ply/ply/lex.py, components/script/dom/bindings/codegen/run.py, components/script/build.rs and 4 more
  • @wafflespeanut: python/requirements.txt
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/codegen/ply/README, components/script/dom/bindings/codegen/ply/ply/lex.py, components/script/dom/bindings/codegen/run.py, components/script/build.rs and 4 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 28, 2020
@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@ghostd
Copy link
Contributor Author

ghostd commented Jan 2, 2021

I updated and rebased the PR. My last issue needs a WPT fix (and is specific to Python 3.9)

I can run bootstrap, build, test-wpt, fmt and test-tidy.

@ghostd ghostd marked this pull request as ready for review January 2, 2021 13:58
@jdm
Copy link
Member

jdm commented Jan 2, 2021

Running the WPT lint on ./tests/wpt/web-platform-tests/...
{"path": "import-maps/data-driven/tools/format_json.py", "lineno": 15, "rule": "PARSE-FAILED", "message": "Unable to parse file"}

This is an interesting failure.

@jdm
Copy link
Member

jdm commented Jan 2, 2021

@jdm
Copy link
Member

jdm commented Jan 3, 2021

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 5a7da27 with merge c8b57e6...

bors-servo added a commit that referenced this pull request Jan 3, 2021
Port some code to Python3

With this PR i wish to share modifications to run `mach` with `./mach` instead of `python3 ./mach`.

I'm using a Debian machine. The following commands work for me:
* `./mach build --dev`
* `./mach test-tidy`
* `./mach fmt`

I still have errors with the WPT scripts, but maybe the root causes is a bad resolution of `requirements.txt`

I used `2to3` to migrate some code before applying some manual fixes. For PLY, I copied/paste the code from the github repository.

If someone can test...
For now i'm not sure how to continue the migration while i can't run WPT :-/

---
<!-- 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
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

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

@jdm jdm left a comment

Choose a reason for hiding this comment

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

The code changes look fine to me.

@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 3, 2021
@jdm
Copy link
Member

jdm commented Jan 3, 2021

We should update https://github.com/servo/servo/blob/master/etc/taskcluster/decision_task.py to always use python3.

@ghostd
Copy link
Contributor Author

ghostd commented Jan 4, 2021

Running the WPT lint on ./tests/wpt/web-platform-tests/...
{"path": "import-maps/data-driven/tools/format_json.py", "lineno": 15, "rule": "PARSE-FAILED", "message": "Unable to parse file"}

This is an interesting failure.

I proposed a PR to fix it

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jan 4, 2021
@ghostd
Copy link
Contributor Author

ghostd commented Jan 4, 2021

We should update https://github.com/servo/servo/blob/master/etc/taskcluster/decision_task.py to always use python3.

I added python3 for each mach command and tried to add a with_python3 method for the Windows build (but i have no clue if it works). This commit really needs a careful review :-)

Comment on lines 654 to 655
python -m ensurepip
pip install virtualenv==20.2.1
Copy link
Member

Choose a reason for hiding this comment

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

I think these may need to be python3 and pip3 respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.with_early_script("""
python -m ensurepip
pip install virtualenv==20.2.1
""")
Copy link
Member

Choose a reason for hiding this comment

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

I believe this also requires .with_path_from_homedir("python3", "python3\\Scripts")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jdm jdm mentioned this pull request Jan 24, 2021
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #28094) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jan 26, 2021
@jdm jdm mentioned this pull request Jan 26, 2021
@tomoverlund
Copy link

How did you get ./mach build --dev without changes to mozjs?

@jdm
Copy link
Member

jdm commented Jan 28, 2021

Presumably it was still using python2 for the mozjs build.

@tomoverlund
Copy link

Presumably it was still using python2 for the mozjs build.

Oh, that makes sense. I'm trying to build in Debian unstable with no python2 in sight.

@ghostd
Copy link
Contributor Author

ghostd commented Jan 29, 2021

I rebased and squashed the commit. The WPT PR has been merged \o/

@jdm
Copy link
Member

jdm commented Feb 2, 2021

I'm working on incorporating these changes into #28092.

@jdm jdm mentioned this pull request Feb 4, 2021
bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
@jdm
Copy link
Member

jdm commented Feb 4, 2021

I'm going to attempt to wrestle CI into shape in #28141.

bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
bors-servo added a commit that referenced this pull request Feb 4, 2021
Port CI to Python3

Based on the work from #27999 and #28092.
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #28147) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Feb 6, 2021

I'm moving ahead with merging this code as part of #28092, because my attempts to get Windows CI working in #28141 didn't work out. I'm going to go ahead and close this PR—thank you so much for doing this work! It was a big help.

@jdm jdm closed this Feb 6, 2021
@ghostd ghostd deleted the migrate-to-py3 branch February 21, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants