Skip to content

tests: cargo clean before checking stubs compile #1445

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

Merged
merged 2 commits into from
Feb 8, 2022
Merged

tests: cargo clean before checking stubs compile #1445

merged 2 commits into from
Feb 8, 2022

Conversation

petertseng
Copy link
Member

1.59.0-beta.6 (0426998f5 2022-02-02) runs into an internal compiler
error unless we do this.
(This has been reported to rustc in
rust-lang/rust#93131)

It may be a good idea to keep this clean in the long term anyway, so
as to ensure that nothing from the "Check exercises" step unduly affects
the "Ensure stubs compile" step.

1.59.0-beta.6 (0426998f5 2022-02-02) runs into an internal compiler
error unless we do this.
(This has been reported to rustc in
rust-lang/rust#93131)

It may be a good idea to keep this `clean` in the long term anyway, so
as to ensure that nothing from the "Check exercises" step unduly affects
the "Ensure stubs compile" step.
(need to make any arbitrary change so that this exercise is tested in PR)
status=1
continue
fi
cargo clean --manifest-path "$ex/Cargo.toml" --package "$name"
Copy link
Member Author

Choose a reason for hiding this comment

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

given it only cleans the exercise package and not dependencies, I would understand objections to calling this "clean_all", in which case I would have to think of a better name. cargo_clean_exercises?

@coriolinus
Copy link
Member

On the one hand this should fix the current CI issues. On the other hand, it definitely slows things down, and fast CI is important for everyone.

I believe the linked issue claims that the underlying incremental compilation issue is fixed in the Rust 1.59 release, coming out in (at the time of writing) 17 days.

I am reluctant to permanently degrade CI times for an issue that is likely to resolve in under three weeks. I am unwilling to claim that I will remember to revert this in three weeks, if merged.

Conclusion: I am entirely uncertain as to whether this is a good idea. I reply to acknowledge that I need to think about this, and will attempt to do so in the near future.

@petertseng
Copy link
Member Author

Better to quantify the slowdown. Opening a demonstrative PR where all exercises are compiled.

Under ideal circumstances, the slowdown is exactly as much as would have otherwise been necessary. Because the src/lib.rs has changed from the previous compilation, it should have been necessary to recompile that in any case. In cleaning only the exercise package and crucially not any others, dependencies are not recompiled, and the hope was that there was no need to recompile those.

@petertseng
Copy link
Member Author

given my seeing no appreciable difference between #1446 and a successful build on main, I think I'd go so far as to recommend making this permanent - not even reverting it when 1.59.0 is released.

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks for this! I agree with the methodology of the test performed and the result obtained. We don't need to remember to revert this.

I don't have any problem with the name of the ci step.

@petertseng petertseng merged commit 4417a29 into exercism:main Feb 8, 2022
@petertseng petertseng deleted the clean branch February 8, 2022 16:48
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