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

Use a consistent profile between 'build' and 'install' #47

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Dec 5, 2024

Choose 'dev' as the default profile (to align with 'build'). Of course the developer can override this with --cargo-args --profile release similar to how CMake developers use --cmake-args -DCMAKE_BUILD_TYPE=Release.

Right now, 'build' and 'install' use 'dev' and 'release' respectively, meaning that you always compile everything twice.

Choose 'dev' as the default profile (to align with 'build'). Of course
the developer can override this with `--cargo-args --profile release`.
@cottsay cottsay added the bug Something isn't working label Dec 5, 2024
@cottsay cottsay self-assigned this Dec 5, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.20%. Comparing base (725167f) to head (a962d49).

Files with missing lines Patch % Lines
colcon_cargo/task/cargo/build.py 75.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   67.21%   67.20%   -0.02%     
==========================================
  Files           6        6              
  Lines         241      247       +6     
  Branches       35       37       +2     
==========================================
+ Hits          162      166       +4     
  Misses         57       57              
- Partials       22       24       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cottsay
Copy link
Member Author

cottsay commented Dec 5, 2024

Are there any other ways of changing the profile that we should test with? Maybe in a global configuration file, or in a project's configuration file?

@luca-della-vedova
Copy link
Member

I forgot to mention, should we include the same fix in the test verb? Since in cargo test actually builds tests before running. It's not a huge deal since it defaults to debug builds but still.

@luca-della-vedova
Copy link
Member

Are there any other ways of changing the profile that we should test with? Maybe in a global configuration file, or in a project's configuration file?

What do you mean by this? What I was defaulting to was just using the cargo-args parameter in the colcon command invocation and, once this library is distributed more widely, adding it to the build-type mixin to reduce user typing

@cottsay
Copy link
Member Author

cottsay commented Dec 5, 2024

should we include the same fix in the test verb? Since in cargo test actually builds tests before running.

This is a little tricky, actually. I'd say that colcon users may not be expecting to be performing any compilation during the test verb invocation, and that it actually breaks patterns employed in CI jobs on the buildfarm. If we continue to perform compilation during test verb, it would be nice if we could detect what profile was used during build and continue to use that. Let's punt it for now.

What do you mean by this? What I was defaulting to was just using the cargo-args parameter in the colcon command invocation and, once this library is distributed more widely, adding it to the build-type mixin to reduce user typing

I don't mean changing it in colcon, I mean changing it in cargo itself. Can the default profile be set in a Cargo.toml? Or in $CARGO_HOME/config.toml? Or an environment variable? I don't think we want to override the developer's configurations here, we just want the two invocations to be consistent.

@cottsay
Copy link
Member Author

cottsay commented Dec 5, 2024

I don't think we want to override the developer's configurations here, we just want the two invocations to be consistent.

To that end, if we had a reliable way to detect the profile that was used by 'build' and then specifically tell 'install' to do the same thing, we should consider it.

@luca-della-vedova
Copy link
Member

I don't think we want to override the developer's configurations here, we just want the two invocations to be consistent.

To that end, if we had a reliable way to detect the profile that was used by 'build' and then specifically tell 'install' to do the same thing, we should consider it.

This is sadly out of my comfort zone and I'm not aware of a way to do it. Still this PR is an incremental improvement for the majority of workflows so I'll approve it first. If any other maintainer knows of a way to detect the profile (or whether there are commonly used ways to override the build type at the project level that we need to detect) feel free to pitch in!

@luca-della-vedova luca-della-vedova merged commit 48a4245 into main Dec 6, 2024
17 checks passed
@luca-della-vedova luca-della-vedova deleted the cottsay/consistent-profile branch December 6, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants