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

Update cargo #9314

Merged
merged 2 commits into from Feb 4, 2016
Merged

Update cargo #9314

merged 2 commits into from Feb 4, 2016

Conversation

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jan 14, 2016

r? @Manishearth

(wait for a try run first - I can't repro the -fPIC stuff from the previous one right now, but we'll see...)

Review on Reviewable

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jan 14, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 14, 2016

Trying commit 246c321 with merge 7d338d0...

bors-servo added a commit that referenced this pull request Jan 14, 2016
Update cargo

r? @Manishearth

(wait for a try run first - I can't repro the -fPIC stuff from the previous one right now, but we'll see...)
@Manishearth
Copy link
Member

Manishearth commented Jan 14, 2016

r=me

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jan 14, 2016

This is another attempt at #8375, but I'll actually dig into the problem this time :-)

@bors-servo
Copy link
Contributor

bors-servo commented Jan 14, 2016

💔 Test failed - linux-rel

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jan 14, 2016

Fascinating! Our build machines have fontconfig version 2.11 installed. As you can see in the build.rs here: https://github.com/servo/libfontconfig/blob/master/build.rs, if there is not a system fontconfig >= 2.11.1, we'll compile locally.

So here's where it gets interesting! With an older cargo, we still link fine (these are our CI machines, and it's what we do today, with our ~9/30/2015 cargo). With an updated cargo, though, we do not! The only idea I have is that maybe older cargo had some sort of bug around still adding system directories to the search path and picking up libfontconfig.a that was version 2.11 instead of the newly built one anyway.

I will keep looking into this today/tomorrow. CC @alexcrichton, since I assume he'll be morbidly curious what I find out :-)

Also CC @AutomatedTester since he's been hitting this on his machine.

@alexcrichton
Copy link
Contributor

alexcrichton commented Jan 14, 2016

Oh dear that does seem interesting... I guess the most useful output would be the build script of the fontconfig-sys crate which may indicate how pkg-config either succeeded or failed?

@AutomatedTester
Copy link

AutomatedTester commented Jan 15, 2016

I think @andreastt was hitting this ( I havent built servo in a while 😄 )

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jan 15, 2016

@AutomatedTester Sorry about the mis-mention! My "IRC nick to GH username" map is not great :-/

@andreastt
Copy link
Contributor

andreastt commented Jan 15, 2016

If my bisecting is correct (I had to skip a few commits that didn’t compile), the bad commit in cargo is rust-lang/cargo@8995f30:

% git bisect bad
8995f3031e232f3f3682394f24905b3296f6c380 is the first bad commit
commit 8995f3031e232f3f3682394f24905b3296f6c380
Author: Alex Crichton
Date:   Wed Sep 30 17:58:08 2015 -0700

    Fix tests on nightly

    Some output of failing tests have been tweaked, so update the associated tests.

:040000 040000 fcf1af9601e866da0bcfd14ef7b4bfa2410fe6af a8260e6e9f7c80a4b148c9ddcd2e3a2317a34bc6 M  tests

I haven’t had time to look at that changeset yet.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Jan 15, 2016

Verbose output from the build is here: https://gist.github.com/larsbergstrom/54d9fc2169ff93896a12

It appears that -fPIC is part of CFLAGS, so not sure what's going on here. Continuing to dig...

@andreastt
Copy link
Contributor

andreastt commented Jan 15, 2016

Sorry, disregard my comment above about the sinning commit in cargo, as it only contained test changes.

I ran my bisect again, not skipping any commits, and I ended up hitting rust-lang/cargo@659f824, which… unfortunately is not entirely trivial.

% git bisect good
659f824406313da0fcaf358708cc77d56a17dba8 is the first bad commit
commit 659f824406313da0fcaf358708cc77d56a17dba8
Date:   Thu Oct 1 23:48:47 2015 -0700
@alexcrichton
Copy link
Contributor

alexcrichton commented Jan 15, 2016

Hm I'm not 100% sure how that would lead to the bug being seen here (or the regression more accurately), but I suspect that understanding what's going on under the hood would also help

@larsbergstrom larsbergstrom mentioned this pull request Jan 20, 2016
52 of 52 tasks complete
@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 2, 2016

@alexcrichton So. I may have a "fix" for this problem, but it confuses me a bit. Can you provide some insight?

If I remove the line:
https://github.com/servo/libfontconfig/blob/master/build.rs#L23
(println!("cargo:rustc-link-lib=static=fontconfig");), then the build succeeds with a newer cargo.

I noticed from the verbose build logs that we were getting an additional -l fontconfig passed to the final linker commandline with a newer cargo which was not present in the previous version of cargo, which suggested trying out this change.

I'm not really sure how that leads to this error, though it can't be good that there's both a static copy of fontconfig that we've built being linked into an rlib PLUS another copy of fontconfig being linked in, which is either coming from the same build directory OR from the link lib search path (I have no idea which one).

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 2, 2016

Based on IRC, I went to look for other places where we also attempt to link in fontconfig. There was another place, in rust-azure's linkhack.rs. If I remove that line but leave in the rustc-link-lib line above, I no longer have the extra -l fontconfig in the output, but still get the errors about needing to recompile with -fPIC.

@alexcrichton
Copy link
Contributor

alexcrichton commented Feb 2, 2016

After some discussion on IRC, we managed to track this down to a cargo bug: rust-lang/cargo#2354

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Trying commit 4b8d5e2 with merge cbaac99...

bors-servo added a commit that referenced this pull request Feb 3, 2016
Update cargo

r? @Manishearth

(wait for a try run first - I can't repro the -fPIC stuff from the previous one right now, but we'll see...)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9314)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💔 Test failed - gonk

@Manishearth
Copy link
Member

Manishearth commented Feb 3, 2016

@bors-servo try- retry p=10

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 3, 2016

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #9505
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

📌 Commit 2ded261 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Testing commit 2ded261 with merge 1a150bd...

bors-servo added a commit that referenced this pull request Feb 3, 2016
Update cargo

r? @Manishearth

(wait for a try run first - I can't repro the -fPIC stuff from the previous one right now, but we'll see...)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9314)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💔 Test failed - linux-rel

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 3, 2016

command timed out: 1200 seconds without output wat?

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💔 Test failed - mac-rel-css

@jdm
Copy link
Member

jdm commented Feb 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💔 Test failed - mac-rel-css

@KiChjang
Copy link
Member

KiChjang commented Feb 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-unit are reusable. Rebuilding only mac-rel-css, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

💔 Test failed - mac-rel-css

@jdm
Copy link
Member

jdm commented Feb 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

Testing commit 2ded261 with merge 44b7ebf...

bors-servo added a commit that referenced this pull request Feb 3, 2016
Update cargo

r? @Manishearth

(wait for a try run first - I can't repro the -fPIC stuff from the previous one right now, but we'll see...)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9314)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 4, 2016

@bors-servo bors-servo merged commit 2ded261 into servo:master Feb 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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