rustc: Swap link order of native libs/rust deps #28605

Merged
merged 1 commit into from Oct 1, 2015

Conversation

Projects
None yet
6 participants
@alexcrichton
Member

alexcrichton commented Sep 23, 2015

This commit swaps the order of linking local native libraries and upstream
native libraries on the linker command line. Detail of bugs this can cause can
be found in #28595, and this change also invalidates the test case that was
added for #12446 which is now considered a bug because the downstream dependency
would need to declare that it depends on the native library somehow.

Closes #28595
[breaking-change]

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 23, 2015

Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Sep 23, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 23, 2015

Member

r? @brson

To be clear, this is going to be a breaking change (as evidenced by the test case that needed to be removed), but I also believe this is a bug fix to correct the test case that was added (e.g. that should definitely work). If we decide to move forward then this should certainly be mentioned in the release notes, and I can write up a longer explanation about what errors might show up and how to fix them.

Member

alexcrichton commented Sep 23, 2015

r? @brson

To be clear, this is going to be a breaking change (as evidenced by the test case that needed to be removed), but I also believe this is a bug fix to correct the test case that was added (e.g. that should definitely work). If we decide to move forward then this should certainly be mentioned in the release notes, and I can write up a longer explanation about what errors might show up and how to fix them.

@rust-highfive rust-highfive assigned brson and unassigned pcwalton Sep 23, 2015

@brson brson added the relnotes label Sep 28, 2015

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Sep 28, 2015

Contributor

Should we crater this? Sounds frightening.

Contributor

brson commented Sep 28, 2015

Should we crater this? Sounds frightening.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Sep 28, 2015

Contributor

@brson Yes. Ok, I'll do it.

Contributor

brson commented Sep 28, 2015

@brson Yes. Ok, I'll do it.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Sep 29, 2015

Contributor

Still running.

Contributor

brson commented Sep 29, 2015

Still running.

@brson

This comment has been minimized.

Show comment
Hide comment
Contributor

brson commented Sep 30, 2015

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Sep 30, 2015

Contributor

@bors r+

Contributor

brson commented Sep 30, 2015

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 30, 2015

Contributor

📌 Commit 0acd11e has been approved by brson

Contributor

bors commented Sep 30, 2015

📌 Commit 0acd11e has been approved by brson

bors added a commit that referenced this pull request Sep 30, 2015

Auto merge of #28605 - alexcrichton:link-native-first, r=brson
This commit swaps the order of linking local native libraries and upstream
native libraries on the linker command line. Detail of bugs this can cause can
be found in #28595, and this change also invalidates the test case that was
added for #12446 which is now considered a bug because the downstream dependency
would need to declare that it depends on the native library somehow.

Closes #28595
[breaking-change]
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 30, 2015

Contributor

⌛️ Testing commit 0acd11e with merge baf8021...

Contributor

bors commented Sep 30, 2015

⌛️ Testing commit 0acd11e with merge baf8021...

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 30, 2015

Contributor

💔 Test failed - auto-linux-64-x-android-t

Contributor

bors commented Sep 30, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Oct 1, 2015

@bors: r=brson 27f0c78

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2015

Contributor

⌛️ Testing commit 27f0c78 with merge 26a1df3...

Contributor

bors commented Oct 1, 2015

⌛️ Testing commit 27f0c78 with merge 26a1df3...

bors added a commit that referenced this pull request Oct 1, 2015

Auto merge of #28605 - alexcrichton:link-native-first, r=brson
This commit swaps the order of linking local native libraries and upstream
native libraries on the linker command line. Detail of bugs this can cause can
be found in #28595, and this change also invalidates the test case that was
added for #12446 which is now considered a bug because the downstream dependency
would need to declare that it depends on the native library somehow.

Closes #28595
[breaking-change]
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2015

Contributor

💔 Test failed - auto-linux-64-x-android-t

Contributor

bors commented Oct 1, 2015

💔 Test failed - auto-linux-64-x-android-t

rustc: Swap link order of native libs/rust deps
This commit swaps the order of linking local native libraries and upstream
native libraries on the linker command line. Detail of bugs this can cause can
be found in #28595, and this change also invalidates the test case that was
added for #12446 which is now considered a bug because the downstream dependency
would need to declare that it depends on the native library somehow.

Closes #28595
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Oct 1, 2015

@bors: r=brson 9502df5

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 1, 2015

Contributor

⌛️ Testing commit 9502df5 with merge 587be42...

Contributor

bors commented Oct 1, 2015

⌛️ Testing commit 9502df5 with merge 587be42...

bors added a commit that referenced this pull request Oct 1, 2015

Auto merge of #28605 - alexcrichton:link-native-first, r=brson
This commit swaps the order of linking local native libraries and upstream
native libraries on the linker command line. Detail of bugs this can cause can
be found in #28595, and this change also invalidates the test case that was
added for #12446 which is now considered a bug because the downstream dependency
would need to declare that it depends on the native library somehow.

Closes #28595
[breaking-change]

@bors bors merged commit 9502df5 into rust-lang:master Oct 1, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
@Ms2ger

This comment has been minimized.

Show comment
Hide comment
Contributor

Ms2ger commented Oct 1, 2015

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 1, 2015

Member

Indeed thanks @Ms2ger! I'll also cc @metajack, and I've created a post on internals about this change to help get ahead of any surprising breakage.

Member

alexcrichton commented Oct 1, 2015

Indeed thanks @Ms2ger! I'll also cc @metajack, and I've created a post on internals about this change to help get ahead of any surprising breakage.

@alexcrichton alexcrichton deleted the alexcrichton:link-native-first branch Oct 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment