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

Avoid ./mach build to clobber the build by default. #20491

Closed
wants to merge 2 commits into from

Conversation

@emilio
Copy link
Member

emilio commented Mar 31, 2018

Fixes #20489.


This change is Reviewable

emilio added 2 commits Mar 31, 2018
@highfive
Copy link

highfive commented Mar 31, 2018

Heads up! This PR modifies the following files:

@emilio
Copy link
Member Author

emilio commented Mar 31, 2018

r? @jdm

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented Mar 31, 2018

Trying commit d2cdfff with merge c7bcd16...

bors-servo added a commit that referenced this pull request Mar 31, 2018
Avoid ./mach build to clobber the build by default.

Fixes #20489.

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

emilio commented Mar 31, 2018

Not sure if the WITH_GOLD bit will make the ARM build fail... I guess we'll see.

@jdm
Copy link
Member

jdm commented Mar 31, 2018

Not sure if the tradeoff is worth disabling the gold linker by default :/

@bors-servo
Copy link
Contributor

bors-servo commented Mar 31, 2018

@emilio
Copy link
Member Author

emilio commented Mar 31, 2018

Not sure if the tradeoff is worth disabling the gold linker by default :/

It definitely is for me, but if you think otherwise I can try to find another solution... Not clear to me how'd that look.

@jdm jdm removed their assignment Apr 3, 2018
@jdm
Copy link
Member

jdm commented Apr 3, 2018

It might be worth taking this to the mailing list. I'm on leave this month, so I'm unassigning myself.

@nox
Copy link
Member

nox commented Apr 18, 2018

@emilio What's the status on this?

@emilio
Copy link
Member Author

emilio commented Apr 18, 2018

I mailed the list. Not sure if we should proceed here or not, I guess @SimonSapin tried to make the gold bits use the cargo config and hit a cargo bug.

@SimonSapin
Copy link
Member

SimonSapin commented Apr 18, 2018

My hope was to move the "unused extern crate" warning to .cargo/config (and while I’m at it the "enable NEON on ARM" bit too, since .cargo/config can have cfg(…) sections), and keep the linker one set by mach in an env variable because it’s done dynamically based on the presence of ld.gold in $PATH. I was hoping that both sets of flags would be passed to rustc, but instead the env variable overrides the config: rust-lang/cargo#5376

@jdm
Copy link
Member

jdm commented Jun 19, 2018

Given the downsides, I'm going to close this.

@jdm jdm closed this Jun 19, 2018
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.

None yet

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