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

ci: simplify workflow #1458

Merged
merged 1 commit into from
Mar 4, 2021
Merged

ci: simplify workflow #1458

merged 1 commit into from
Mar 4, 2021

Conversation

davidhewitt
Copy link
Member

This extracts just the ci workflow file changes from #1457.

Instead of having separate build & test stages, this now just runs tests for various feature combinations. On pypy this command is overridden to just build --lib --tests.

@kngwyu
Copy link
Member

kngwyu commented Mar 3, 2021

Thanks, but ... 🤔, is it good to use environmental variables to shorten commands?
I'm not familiar with CI good practices, though.

@davidhewitt davidhewitt force-pushed the simplify-ci branch 2 times, most recently from 14bc993 to e5624ca Compare March 4, 2021 08:18
@davidhewitt
Copy link
Member Author

I've pushed a commit to use "outputs" of a github actions step, maybe you prefer that?

I wanted to use variables of some form because it simplifies maintenance of the CI scripts - every time we add a new feature, there were a lot of places to edit. Also the various cargo commands were getting inconsistent with PyPy. Now the structure is more consistent on all CI runs, and there's only one place to edit if we change a new feature.

@kngwyu
Copy link
Member

kngwyu commented Mar 4, 2021

I've pushed a commit to use "outputs" of a github actions step, maybe you prefer that?

I'm sorry but I'm thinking that Github actions might provide something better than env...
But it looks like that env is what we want for all-additive-features: https://docs.github.com/en/actions/reference/environment-variables

@davidhewitt
Copy link
Member Author

Ah ok, I'll revert that last commit? Always good to try things 😄

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Mar 4, 2021

I noticed that we can use with instead of env: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepswith, but... I really don't know which is better. They look the same 😕

build.rs Outdated
@@ -618,7 +618,11 @@ fn get_rustc_link_lib(config: &InterpreterConfig) -> String {
// See https://www.python.org/dev/peps/pep-0384/#linkage
//
// This contains only the limited ABI symbols.
if env::var_os("CARGO_FEATURE_ABI3").is_some() {
if env::var_os("CARGO_FEATURE_ABI3").is_some()
// FIXME: PyPy doesn't currently ship a stable ABI dll.
Copy link
Member

@kngwyu kngwyu Mar 4, 2021

Choose a reason for hiding this comment

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

Isn't it better to abort the build with some warnings?

@kngwyu
Copy link
Member

kngwyu commented Mar 4, 2021

I think we can only refactor all-additive-features and leave the PyPy conditioning as is. Since we cannot build the crate with PyPy and abi3, I don't see any strong motivation to commonize them.

@davidhewitt
Copy link
Member Author

Ok will push a commit to rework this PR later today. Thanks for looking at this with me 👍

@davidhewitt
Copy link
Member Author

davidhewitt commented Mar 4, 2021

I'm gonna merge this so that I can rebase #1457 on it. Hope that's alright.

(If there are changes you want, I'm happy to do them in a follow-up PR.)

@davidhewitt davidhewitt merged commit 525358a into PyO3:master Mar 4, 2021
@davidhewitt davidhewitt deleted the simplify-ci branch August 10, 2021 07:20
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