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
New folder linux #12850
New folder linux #12850
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon. |
Heads up! This PR modifies the following files:
|
r? @aneeshusa |
@ConnorGBrewster Is this what you were looking expecting? |
Yes, this is what I had in mind. Although I will let @aneeshusa review this still, since I don't know all of our python best practices. |
@ConnorGBrewster Perfect! That makes sense. :) I apologize for the delay in sending the PR. |
No worries! Thanks for doing it! 😄 |
@@ -209,24 +209,21 @@ def package(self, release=False, dev=False, android=None, debug=False, debugger= | |||
msi_path = path.join(dir_to_msi, "Servo.msi") | |||
print("Packaged Servo into {}".format(msi_path)) | |||
else: | |||
dir_to_temp = '/'.join(binary_path.split('/')[:-2]) + '/targz' | |||
dir_to_package = '/'.join(binary_path.split('/')[:-1]) | |||
dir_to_root = '/'.join(binary_path.split('/')[:-3]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dir_to_root is no longer used.
Sorry about the delay in reviewing! Please also try to run Servo from the package tarball to make sure the packaging works properly. |
@Coder206 How is this going? Do you have any questions? |
print("Copying files") | ||
shutil.copytree(dir_to_package + '/resources', resources_dir) | ||
shutil.copytree(browserhtml_path, resources_dir + browserhtml_path.split('/')[-1]) | ||
|
||
print("Writing runservo.sh") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aneeshusa Is it still necessary to make a runservo.sh here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ConnorGBrewster has a PR to remove it: #12154.
It looks like it would be pretty easy to fold that PR into this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should I work to combine the two PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just realized that removing runservo.sh
will require some additional changes, so let's keep runservo.sh
for now and handle the removal in the other PR.
@bors-servo r+ |
📌 Commit be87452 has been approved by |
New folder linux <!-- Please describe your changes on the following line: --> --- <!-- 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 - [x] These changes fix #11983 (github issue number if applicable). - [x] There are tests for these changes (./mach package) <!-- 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/12850) <!-- Reviewable:end -->
💔 Test failed - linux-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always remember to run ./mach test-tidy
before pushing :)
# TODO(aneeshusa): lock dir_to_temp to prevent simultaneous builds | ||
print("Cleaning up from previous packaging") | ||
delete(dir_to_temp) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line has whitespace, remove it (line should just be empty).
The purpose of the code is to make a new temporary folder with the necessary resources used to package Linux. Before this, the package was built based on a modified target directory. By using a temporary folder, it removes the need to rebuild Servo every time it gets packaged for Linux.
@bors-servo r=aneeshusa |
📌 Commit d672750 has been approved by |
New folder linux <!-- Please describe your changes on the following line: --> --- <!-- 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 - [x] These changes fix #11983 (github issue number if applicable). - [x] There are tests for these changes (./mach package) <!-- 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/12850) <!-- Reviewable:end -->
☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev |
Thank you very much @aneeshusa! |
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is