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

deny warnings #19612

Merged
merged 1 commit into from Dec 21, 2017
Merged

deny warnings #19612

merged 1 commit into from Dec 21, 2017

Conversation

@tigercosmos
Copy link
Collaborator

tigercosmos commented Dec 20, 2017

deny warnings
related to #19573


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19572 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Dec 20, 2017

Heads up! This PR modifies the following files:

@emilio
Copy link
Member

emilio commented Dec 20, 2017

r? @jdm

This looks reasonable to me.

@jdm
Copy link
Member

jdm commented Dec 20, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

📌 Commit 20c0564 has been approved by jdm

@SimonSapin
Copy link
Member

SimonSapin commented Dec 20, 2017

@bors-servo r-

Please also remove #![deny(warnings)] where it exists (which this should replace), and also add RUSTFLAGS on Buildbot for other configurations which could have platform-specific code. (Windows, ARM Linux, Android)

@SimonSapin
Copy link
Member

SimonSapin commented Dec 20, 2017

Thanks for your work on this!

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 20, 2017

@highfive highfive assigned SimonSapin and unassigned jdm Dec 20, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Dec 20, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

📌 Commit 0959445 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

Testing commit 0959445 with merge a73bd82...

bors-servo added a commit that referenced this pull request Dec 20, 2017
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19572 (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. -->

<!-- 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/19612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

💔 Test failed - arm32

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 20, 2017

It's weird.

it should be

env ANDROID_SDK=/home/servo/android/sdk/r25.2.3 RUSTFLAGS='-D warnings' ./mach build --android --dev

but it becomes

env ANDROID_SDK=/home/servo/android/sdk/r25.2.3 'RUSTFLAGS='"'"'-D' 'warnings'"'"'' ./mach build --android --dev
@jdm
Copy link
Member

jdm commented Dec 20, 2017

If you use -Dwarnings then you won't need any quote characters.

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 20, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

Trying commit e8e828f with merge b8b9b31...

bors-servo added a commit that referenced this pull request Dec 21, 2017
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19572 (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. -->

<!-- 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/19612)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member

SimonSapin commented Dec 21, 2017

 argv: ['RUSTFLAGS=-Dwarnings', './mach', 'build-geckolib']

This indicates that RUSTFLAGS=-Dwarnings is interpreted as the program name. Try adding env at the start of the line: this is a program that sets environment variables and then starts another program.

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 21, 2017

@SimonSapin You are right!
I add env at the beginning. It must work. Hope so..
Could you r?

@jdm
Copy link
Member

jdm commented Dec 21, 2017

When we change RUSTFLAGS between building and compiling unit tests, we have to recompile everything. This makes TravisCI time out, and will make all of our builds take longer.

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 21, 2017

@jdm Ok I will fix it.

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 21, 2017

now it it OK

@@ -23,7 +23,6 @@
//! [cssparser]: ../cssparser/index.html
//! [selectors]: ../selectors/index.html

#![deny(warnings)]

This comment has been minimized.

Copy link
@jdm

jdm Dec 21, 2017

Member

Let's leave these explicit instances of denying warnings, since developers have grown used to developing with them present.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 21, 2017

Member

@jdm Shouldn’t we rather be consistent between crates? As far as I’m concerned it’s an historical accident that some crates have it and not others, not something useful.

This comment has been minimized.

Copy link
@emilio

emilio Dec 21, 2017

Member

I don't personally care, but at least nsstring is vendored code, so I'd rather leave it in that crate at least.

@jdm
Copy link
Member

jdm commented Dec 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

📌 Commit 6d6491e has been approved by jdm

@highfive highfive assigned jdm and unassigned SimonSapin Dec 21, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

Testing commit 6d6491e with merge 2eb1512...

bors-servo added a commit that referenced this pull request Dec 21, 2017
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19572 (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. -->

<!-- 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/19612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 21, 2017

@bors-servo bors-servo merged commit 6d6491e into servo:master Dec 21, 2017
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
@tigercosmos tigercosmos deleted the tigercosmos:o1 branch Dec 22, 2017
@aneeshusa aneeshusa mentioned this pull request Jan 8, 2018
2 of 5 tasks complete
SimonSapin added a commit that referenced this pull request Jan 31, 2018
We already have #19612
to deny warnings at the time of landing into master.
But it’s not useful to break the build when later compiler
with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619
bors-servo added a commit that referenced this pull request Jan 31, 2018
Remove #![deny(warnings)]

We already have #19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619

<!-- 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/19914)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 31, 2018
…); r=nox

We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : ed5c34f72e321ce0051347bd5156389d1ef26bef
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2018
We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30

--HG--
extra : source : bbef24920c2bd12a23b63a100d83e6d240ea74dc
extra : amend_source : dc20fe3c006be58542d64afc137ac0a84f74508d
xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Feb 11, 2018
We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 2, 2019
…); r=nox

We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30

UltraBlame original commit: bbef24920c2bd12a23b63a100d83e6d240ea74dc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…); r=nox

We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30

UltraBlame original commit: bbef24920c2bd12a23b63a100d83e6d240ea74dc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 2, 2019
…); r=nox

We already have servo/servo#19612 to deny warnings at the time of landing into master. But it’s not useful to break the build when later compiler with a more recent Rust version that has introduced new warnings:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434619

Source-Repo: https://github.com/servo/servo
Source-Revision: 7546c37f1e921a112fef5828c59c6738a98c3f30

UltraBlame original commit: bbef24920c2bd12a23b63a100d83e6d240ea74dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.