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

Add libc search noop option #6122

Merged
merged 8 commits into from Jul 15, 2018

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 13, 2018

Problem

Our LibcDev subsystem searches for crti.o, a file necessary to create executables on Linux. We use this in order to test both clang and gcc in test_native_toolchain.py. This is not needed except within Pants CI right now, and if the host system doesn't contain this file, it will always error out.

Solution

  • Add --enable-libc-search to NativeToolchain, defaulting to False, so that systems without this currently-unnecessary file don't need to worry about this implementation detail.
  • Create @platform_specific decorator for individual tests which should only run on a specific platform to validate the expected behavior of this flag.

Result

Pants invocations using the native toolchain on Linux hosts without a crti.o do not error out.

cosmicexplorer added some commits Jul 13, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

pants.ini Outdated
@@ -234,6 +234,9 @@ eslint_setupdir: %(pants_supportdir)s/eslint
eslint_config: %(pants_supportdir)s/eslint/.eslintrc
eslint_ignore: %(pants_supportdir)s/eslint/.eslintignore

[native-toolchain]
enable_libc_search: True

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

If this is only for use in travis (which is maybe a bit odd?) then you could put it in pants.travis-ci.ini.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 13, 2018

Contributor

It is, and it is a bit odd. I'll move it into that file.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 14, 2018

Contributor

Moved in b902a7f, along with a more complete docstring describing why this is necessary.

def register_options(cls, register):
super(NativeToolchain, cls).register_options(register)

register('--enable-libc-search', type=bool, default=False, fingerprint=True, advanced=True,

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

It seems like maybe this should be defined next to --libc-dir, and be mutually exclusive?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 13, 2018

Contributor

That's possible, and wouldn't be too much work. The NativeToolchain has been what is doing most of the platform-specific muxing so it seemed to make sense to decouple that here. I can definitely see this being entirely in the libc subsystem. Not sure if there's an obvious winner.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 13, 2018

Contributor

I guess my concern would just be that this file is 100% necessary to find, currently, on Linux, at least in ci, so disabling it there is effectively saying "I want to throw an error 100% of the time". Not sure I'm able to make that sentence make more sense. If you're thinking it should be in the libc subsystem I'll just do that.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 13, 2018

Contributor

Hm. But then, this flag wouldn't make sense at all on OSX, so it would be weird to have it on the cross-platform subsystem. I'll move it to the libc subsystem.

cosmicexplorer added some commits Jul 14, 2018

move libc.enable_libc_search=True to pants.travis-ci.ini
also expand docstring to explain better what this subsystem is used for currently

@cosmicexplorer cosmicexplorer merged commit 53e6ac3 into pantsbuild:master Jul 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment