Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
rust plugin: make libc configurable #1382
Conversation
| - | ||
| - def __init__(self, expected_arch): | ||
| - self._expected_arch = expected_arch | ||
| +class HasMagic: |
elopio
Jul 4, 2017
Member
Can you please explain this in a comment in the source code?
I'm totally confused because it's called magic.
Ideally, you could find a better name for this that's not the name of the module, which is pretty bad. Something like HasType? HasMIMEType?
| @@ -101,6 +101,18 @@ def test_cross_compiling(self): | ||
| binary = os.path.join(self.parts_dir, 'rust-hello', 'install', 'bin', | ||
| 'rust-hello') | ||
| self.assertThat(binary, HasArchitecture('aarch64')) | ||
| + self.assertThat(binary, HasLinkage('dynamically linked')) | ||
| + | ||
| + def test_cross_compiling_musl(self): |
kalikiana
Sep 11, 2017
Collaborator
I figured the existing ones are enough... I added the linkage assertion to the existing first test now.
| @@ -0,0 +1,14 @@ | ||
| +name: rust-musl |
elopio
Jul 4, 2017
Member
what would you think of using the basic rust yaml, and in the test setup append libc: musl, instead of adding a new snap?
kalikiana
Sep 11, 2017
Collaborator
I guess now that you've also proposed new fixtures, this can be refactored once this lands.
| + | ||
| + | ||
| +class HasLinkage(HasMagic): | ||
| + """Match if the file was built for the expected architecture""" |
kalikiana
self-assigned this
Jul 19, 2017
kalikiana
changed the title from
rust: Make libc configurable
to
rust: make libc configurable
Aug 8, 2017
kalikiana
changed the title from
rust: make libc configurable
to
rust plugin: make libc configurable
Aug 8, 2017
sergiusens
added this to In Progress (Implementation)
in 17.10
Aug 8, 2017
sergiusens
moved this from In Progress (Implementation)
to Container builds
in 17.10
Aug 9, 2017
kalikiana
moved this from Container builds
to Cross compilation
in 17.10
Sep 8, 2017
kyrofa
requested changes
Sep 8, 2017
This is looking pretty good, but in addition to Leo's suggestions, I have a few more.
| - | ||
| - def __init__(self, expected_arch): | ||
| - self._expected_arch = expected_arch | ||
| +class HasBinaryFileHeader: |
kyrofa
Sep 8, 2017
Member
This should probably start with an underscore seeing that it can't be used standalone.
kalikiana
Sep 11, 2017
Collaborator
I changed it so it can be used on its own instead. Also made the subclasses smaller.
| @@ -46,6 +48,7 @@ class Options: | ||
| rust_features = [] | ||
| rust_revision = '' | ||
| rust_channel = '' | ||
| + libc = self.libc if hasattr(self, 'libc') else 'gnu' |
kalikiana
added some commits
Jun 27, 2017
|
Apologies for making a mess... I rebased, which locally got me a clean diff and commits on top of master. Can't see how to fix it for GitHub, though. |
kalikiana
changed the base branch from
rust-musl
to
master
Sep 21, 2017
| @@ -103,6 +107,24 @@ def test_cross_compiling(self): | ||
| binary = os.path.join(self.parts_dir, 'rust-hello', 'install', 'bin', | ||
| 'rust-hello') | ||
| self.assertThat(binary, HasArchitecture('aarch64')) | ||
| + self.assertThat(binary, HasLinkage('dynamically linked')) |
elopio
Sep 22, 2017
Member
It seems to me that the matchers would be clearer called like IsDynamicallyLinked() and IsStaticallyLinked()
kalikiana
Sep 22, 2017
Collaborator
To my mind HasLinkage is consistent with HasArchitecture in that it's not hiding the basic string-parsing nature of the matcher... but yeah, I added subclasses as per your suggestion now.
| @@ -31,6 +31,9 @@ | ||
| - rust-features | ||
| (list of strings) | ||
| Features used to build optional dependencies | ||
| + - libc: | ||
| + (string; default: gnu) | ||
| + Libc to link against. Valid options are: gnu (glibc) and musl |
kalikiana
Sep 22, 2017
Collaborator
gnu and musl are the names of the toolchains. I mention glibc there to clarify what it means.
|
I have a few comments, but no complaints here. What do you think of my suggestions for the matchers without args? I'll give you a +1 now, but I think it would be clearer that way. |
kalikiana
added some commits
Sep 22, 2017
|
stale |
sergiusens
closed this
Oct 9, 2017
|
Why did you close this? I merged master on Thursday to try and get past the false negatives and it had positive reviews. |
kalikiana
reopened this
Oct 11, 2017
|
False negative... |
|
@kalikiana we can't merge something with false negatives, because it prevents the following tests from running. I know it's annoying, but keep retrying. |
|
Closing for now, need to investigate how to make this more reliable on Travis. |
kalikiana commentedJun 27, 2017
rust supports musl as a libc alternative to glibc and it's especially interesting for static linking.