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

[test] Check LD_LIBRARY_PATH a little less strictly. #2976

Merged
merged 1 commit into from Jun 21, 2016

Conversation

jrose-apple
Copy link
Contributor

LD_LIBRARY_PATH is one of the few environment variables the LLVM 'lit' tool doesn't strip out, presumably because on some Linux systems it's necessary to run some of the built products being tested. However, the Swift driver also uses LD_LIBRARY_PATH when providing -L options to the script interpreter on Linux. Weaken some of our tests when there's an LD_LIBRARY_PATH in the environment.

(There are similar environment variables on OS X, but we don't have to do anything special there because lit does strip those out. This is presumably okay because all of LLVM's load-time dependencies on OS X are in standard system locations.)

Fixes SR-813.


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

LD_LIBRARY_PATH is one of the few environment variables the LLVM 'lit'
tool /doesn't/ strip out, presumably because on some Linux systems
it's necessary to run some of the built products being tested.
However, the Swift driver also uses LD_LIBRARY_PATH when providing -L
options to the script interpreter on Linux. Weaken some of our tests
when there's an LD_LIBRARY_PATH in the environment.

(There are similar environment variables on OS X, but we don't have to
do anything special there because lit /does/ strip those out. This is
presumably okay because all of LLVM's load-time dependencies on OS X
are in standard system locations.)

https://bugs.swift.org/browse/SR-813
@jrose-apple
Copy link
Contributor Author

@modocache, does this seem like a reasonable workaround? I mocked up the original situation on OS X and this does seem to fix it.

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

Some other potential Linux reviewers: @hpux735, @rintaro, @gribozavr.

@modocache
Copy link
Collaborator

Seems like a fine solution to me!

Also /cc @mikehenson, who reported SR-813.

@hpux735
Copy link
Contributor

hpux735 commented Jun 20, 2016

I'll compile and test on ARM momentarily, but I don't anticipate any issues.

@gribozavr
Copy link
Collaborator

LGTM, waiting for @hpux735.

@hpux735
Copy link
Contributor

hpux735 commented Jun 21, 2016

Go ahead and merge. There's something else wrong with testing on ARM, and I don't want to hold this up.

@gribozavr gribozavr merged commit 3a6a2da into apple:master Jun 21, 2016
@gribozavr
Copy link
Collaborator

@hpux735 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants