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

Expose 'trust_ci' as a feature flag under CI #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Susurrus
Copy link

$CI was chosen instead of $TRAVIS and the ! -z check was done such that
this can work on all CI platforms (at least GitLab CI, Travis CI, AppVeyor,
and Circle CI).

@japaric
Copy link
Owner

japaric commented Apr 19, 2017

This seems fine to me but:

  • ci should be renamed to something like trust-ci to reduce the chances of that cfg clashing from something the crate is already using.

  • I'm not sure if RUSTFLAGS is whitelisted and passed down to the docker containers, can you confirm this works by e.g. adding a test to src/lib.rs that would fail is executed but that has the cfg_attr(ci, ignore) attribute?

@Susurrus
Copy link
Author

I think this is what you were asking for. I should note that I was expecting cross-rs/cross#91 to block this as then we can add a default whitelist for RUSTFLAGS to a template Cross.toml.

@Susurrus Susurrus changed the title Expose 'ci' as a feature flag under CI Expose 'trust_ci' as a feature flag under CI Apr 19, 2017
@japaric
Copy link
Owner

japaric commented Apr 20, 2017

I think this is what you were asking for.

Yes, thanks.

Now it's clear that this needs whitelisting the RUSTFLAGS variable.

@Susurrus
Copy link
Author

Interesting, it actually looks like RUSTFLAGS is being passed through on mac. On Travis only the DISABLE_TEST targets should have passed, but mac also passed and shows the test as properly ignored.

@japaric
Copy link
Owner

japaric commented Apr 20, 2017

Running cross on macOS doesn't use Docker at all (cross doesn't support macOS as a host) so the cross build environment on mac is the same as the Travis environment.

@Susurrus
Copy link
Author

Getting back to this now that cross-rs/cross#91 is merged, I'm actually uncertain how to set up Cross.toml for these tests in order to pass through RUSTFLAGS.

Additionally, should this whitelist RUSTFLAGS by default by having a Cross.toml set up with it and suggesting people copy that as well?

@japaric
Copy link
Owner

japaric commented Jun 11, 2017

I think whitelisting RUSTFLAGS by default in Cross, itself, makes sense.

As for avoiding copying around a Cross.toml in the general case we could use Cargo's convention of making the settings in there configurable via environment variables. For instance, in Cargo land CARGO_BUILD_RUSTFLAGS="foo bar" is equivalent to [build] rustflags = ["foo", "bar"] in .cargo/config. We could have something similar like CROSS_BUILD_ENV_PASSTHROUGH being equivalent to the build.env.passthrough in Cross.toml.

@Susurrus
Copy link
Author

I think I understand. So this would require another change to cross such that it can pull config values from environment variables as you described above. And then in addition to setting RUSTFLAGS in ci/script.rs I'd also set CROSS_BUILD_ENV_PASSTHROUGH to RUSTFLAGS. I don't know how to deal with arrays in envvars, but maybe Cargo's source would be of help here.

The only issue with this is then how does the user override this behavior easily? If they a) don't want to passthrough RUSTFLAGS or b) they want to passthrough more variables. We'd need Cross.toml to append these lists together, which I don't think is what you'd expect for envvars. What were you thinking of in this case?

@japaric
Copy link
Owner

japaric commented Sep 14, 2017

@Susurrus sorry, this PR fell off my radar.

I don't know how to deal with arrays in envvars, but maybe Cargo's source would be of help here.

Cargo uses spaces to split a strings into an array of strings. I think that should work here too since env variables can't have spaces in their names (or at least I think they can't).

The only issue with this is then how does the user override this behavior easily?

Maybe put the CROSS_BUILD_ENV_PASSTHROUGH variable in the env.global field of .travis.yml / AppVeyor.yml and mention what it does and that's it's meant to be user configurable.

If they a) don't want to passthrough RUSTFLAGS

They can comment out the CROSS_BUILD_ENV_PASSTHROUGH variable altogether.

or b) they want to passthrough more variables.

They can manually edit the value of the variable to append other variable names.

ci/script.sh Outdated
@@ -4,6 +4,11 @@ set -ex

# TODO This is the "test phase", tweak it as you see fit
main() {
# Add a cfg spec to allow disabling specific tests under CI.
if [ ! -z $CI ]; then
export RUSTFLAGS=--cfg=trust_ci
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be better to append this flag to RUSTFLAGS here in case the user may want to set RUSTFLAGS in the env.global field.

$CI was chosen instead of $TRAVIS and the ! -z check was done such that
this can work on all CI platforms (at least GitLab CI, Travis CI, AppVeyor,
and Circle CI).
@Susurrus
Copy link
Author

I fixed everything I think, but this relies on changes in Cross such that it will read from environment variables, IIUC. So this will block until I get that functionality added.

@japaric
Copy link
Owner

japaric commented Nov 8, 2017

Opened cross-rs/cross#147 to track the missing support for reading configuration from the environment.

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.

2 participants