Skip to content

/ servo

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

Jump to bottom

Add a `mach` argument to enable debug assertions #14009

Closed
aneeshusa opened this issue Nov 1, 2016 · 6 comments
Closed

Add a `mach` argument to enable debug assertions #14009

aneeshusa opened this issue Nov 1, 2016 · 6 comments
Labels
A-mach C-assigned E-easy L-python

Comments

@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Nov 1, 2016

Follow up from #13387.

mach currently honors the SERVO_ENABLE_DEBUG_ASSERTIONS environment variable to enable debug assertions. To make this option more discoverable and better documented, add a mach flag that does the same thing.

IMO, we can also remove the code that checks for and acts on the environment variable.

Files:

  • python/servo/build_commands.py
  • etc/ci/buildbot_steps.yml (pass the new flag for the linux-rel-wpt and linux-rel-css builders)

Tests:

  • I can't think of a good automated test, so manual testing is probably fine.

cc @emilio @Ms2ger
@highfive
Copy link

@highfive highfive commented Nov 1, 2016

cc @wafflespeanut
@highfive
Copy link

@highfive highfive commented Nov 1, 2016

Please make a comment here if you intend to work on this issue. Thank you!
@ghost
Copy link

@ghost ghost commented Nov 2, 2016

I will work on this. I have a question. There are two places where SERVO_ENABLE_DEBUG_ASSERTIONS is referenced in python/servo/build_commands.py, in def build and def build_cef. Should the flag be added to both build and build-cef?
@KiChjang KiChjang added the C-assigned label Nov 2, 2016
@jdm
Copy link
Member

@jdm jdm commented Nov 2, 2016

Yep!
@ghost ghost mentioned this issue Nov 2, 2016
Merged
4 of 5 tasks complete
@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Nov 2, 2016

Since we're removing the environment variable, we also need a follow-up PR to saltfs to revert the PR adding the env var to the build environment.
bors-servo added a commit that referenced this issue Nov 5, 2016
@bors-servo
Auto merge of #14025 - iamrohit7:debug-assertions, r=Wafflespeanut
Loading status checks…
77b6c09 
Add --with-debug-assertions flag for Mach

I tested out building a release with the flag and added a `debug_assert!(false)` in `fn main` and it panicked.
<!-- 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 #14009  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they are manually tested

<!-- 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/14025)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 6, 2016
@bors-servo
Auto merge of #14025 - iamrohit7:debug-assertions, r=Wafflespeanut
Loading status checks…
dbce416 
Add --with-debug-assertions flag for Mach

I tested out building a release with the flag and added a `debug_assert!(false)` in `fn main` and it panicked.
<!-- 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 #14009  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they are manually tested

<!-- 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/14025)
<!-- Reviewable:end -->
@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Nov 6, 2016

Still need a follow-up to saltfs.
@aneeshusa aneeshusa reopened this Nov 6, 2016
@aneeshusa aneeshusa removed the C-assigned label Nov 6, 2016
@ghost ghost mentioned this issue Nov 6, 2016
Merged
bors-servo added a commit to servo/saltfs that referenced this issue Nov 6, 2016
@bors-servo
Auto merge of #534 - iamrohit7:debug_assertions, r=Wafflespeanut
Loading status checks…
936cf24 
Revert "Add an environment variable to tell mach to build with debug …

…assertions."

fixes servo/servo#14009

This reverts commit 97d8768. Follow up to servo/servo#14025

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/534)
<!-- Reviewable:end -->
bors-servo added a commit to servo/saltfs that referenced this issue Nov 6, 2016
@bors-servo
Auto merge of #534 - iamrohit7:debug_assertions, r=Wafflespeanut
Loading status checks…
5412161 
Remove the environment variables for debug assertions

…assertions."

fixes servo/servo#14009

This reverts commit 97d8768. Follow up to servo/servo#14025

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/534)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-mach C-assigned E-easy L-python
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Add --with-debug-assertions flag for Mach
Remove the environment variables for debug assertions
5 participants
@jdm @aneeshusa @KiChjang @highfive @wafflespeanut
You can’t perform that action at this time.