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

Add --test={skip|native|native-and-emulated} #1190

Merged
merged 17 commits into from
Nov 15, 2024

Conversation

hadim
Copy link
Contributor

@hadim hadim commented Nov 13, 2024

See #1175 for context.

It adds a new flag to build that allow skipping the tests if cross compiling: --no-test-if-emulate

I have a few questions:

  • should I prefer "emulate" or "cross compiling" for the variables as well as the CLI doc?
  • how should noarch should be handled here? or when should we consider the build is a cross compilation or not?

@wolfv
Copy link
Member

wolfv commented Nov 14, 2024

maybe --only-test-native or some variation like that? Or --no-emulation or --no-test-emulation?

Or we could also make this opt-in, by saying: --has-emulation. Or something like --run-test.

@wolfv
Copy link
Member

wolfv commented Nov 14, 2024

If we make it opt-in, we can print a big fat warning and then say: to run tests in non-native environments, use --run-test or something liek that.

@hadim
Copy link
Contributor Author

hadim commented Nov 14, 2024

Do you prefer this to be opt-in? I originally wanted to keep the default behavior the same but up to you.

  • if we go with opt-in then --run-tests or --run-tests-non-native makes sense to me.
  • if we go opt-out then I like --only-test-native

I guess the question (to decide the best default) is whether "on average" running tests on non-native will work more often than it will fail.

Let me know what you prefer and I will adapt the code.

@baszalmstra
Copy link
Contributor

I think opt-in makes sense here. Looking from the perspective of someone who just wants to invoke rattler-build. I would not expect a big fat error.

@hadim
Copy link
Contributor Author

hadim commented Nov 14, 2024

So, would --run-tests-non-native work for you?

@hadim
Copy link
Contributor Author

hadim commented Nov 14, 2024

Also should I add a test in test_simple.py?

@wolfv
Copy link
Member

wolfv commented Nov 14, 2024

We had a build tools meeting and I brought up this question :)

One suggestion was to copy what conda-forge does: https://conda-forge.org/docs/maintainer/conda_forge_yml/#test

--test=native

--test=native-and-emulated

Although we don't really know when there is an "emulator". We could however potentially infer that if there are any host dependencies configured in a test ...

We could also retire the current --skip-test flag and do

--test=skip
# and maybe the inverse
--test=always

Anyways, just ideas.

@hadim
Copy link
Contributor Author

hadim commented Nov 14, 2024

I like the idea of something more expressive than a boolean flag.

I also think it would be nice to propose something related to emulation/cross-compilation so we don't have to compute that logic "outside" of rattler-build. Even if it's opinionated, we can always finetune and adapt it moving forward.

So, considering all the comments above, we could do:

  • --test=skip : replace --skip-test
  • --test=native: run when host_platform != build_platform + always run target_platform == 'noarch'
  • --test=native-and-emulated: always run tests

We can have --test=native as default, so it's opt-in.

WDYT?

@wolfv
Copy link
Member

wolfv commented Nov 14, 2024

Looks good to me! I still would print a warning if tests are not run when cross compiling though.

@hadim hadim changed the title Add --no-test-if-emulate flag Add --test={skip|native|native-and-emulated} Nov 14, 2024
@hadim
Copy link
Contributor Author

hadim commented Nov 14, 2024

LGTM

@baszalmstra
Copy link
Contributor

Nice!

src/tool_configuration.rs Outdated Show resolved Hide resolved
@wolfv
Copy link
Member

wolfv commented Nov 15, 2024

Excellent work @hadim! I think we need to keep the old flag to not break conda-forge (and we remove it in a few months). The old flag can just override the test strategy to skip, right?

@wolfv
Copy link
Member

wolfv commented Nov 15, 2024

Thanks!

@wolfv wolfv enabled auto-merge (squash) November 15, 2024 12:47
auto-merge was automatically disabled November 15, 2024 12:47

Head branch was pushed to by a user without write access

@wolfv wolfv enabled auto-merge (squash) November 15, 2024 12:51
@wolfv wolfv merged commit 665125d into prefix-dev:main Nov 15, 2024
19 checks passed
@hadim hadim deleted the skip_test_if_emulate branch November 15, 2024 13:24
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.

3 participants