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

twoliter: Dedup cargo make logic #97

Merged
merged 2 commits into from
Oct 11, 2023
Merged

twoliter: Dedup cargo make logic #97

merged 2 commits into from
Oct 11, 2023

Conversation

ecpullen
Copy link
Contributor

@ecpullen ecpullen commented Oct 6, 2023

Issue #, if available:

Part of the work on #82

Description of changes:

We will be calling cargo make from different CLI commands, so we want the cargo make invocation to be re-usable.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ecpullen ecpullen requested a review from webern October 6, 2023 20:27
@ecpullen ecpullen force-pushed the cargo-make branch 3 times, most recently from d81edc6 to 9f928d2 Compare October 6, 2023 23:07
@ecpullen
Copy link
Contributor Author

ecpullen commented Oct 6, 2023

^ Add testing for all CargoMake functions.

twoliter/src/cargo_make.rs Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
Comment on lines +33 to +65
let (sdk, toolchain) = require_sdk(project, &arch.into())?;
Ok(Self::default()
.env("TLPRIVATE_SDK_IMAGE", sdk)
.env("TLPRIVATE_TOOLCHAIN", toolchain))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about whether we should just hang on to &'a Project and do things like this during the exec call. I'm also slightly concerned about lack of clarity regarding what args could be passed during exec and which are managed by this object.

These are not huge concerns. Just some things to consider and change if it feels like we can do better.

@ecpullen ecpullen force-pushed the cargo-make branch 2 times, most recently from 735e75a to 0eb0330 Compare October 10, 2023 21:22
@ecpullen ecpullen requested review from webern and bcressey October 10, 2023 21:22
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/cargo_make.rs Outdated Show resolved Hide resolved
twoliter/src/test/mod.rs Outdated Show resolved Hide resolved
.github/workflows/rust.yml Show resolved Hide resolved
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Ethan said he found a bug. Marking this as do-not-merge.

@ecpullen ecpullen merged commit 01f76b2 into develop Oct 11, 2023
1 check passed
@ecpullen ecpullen deleted the cargo-make branch October 11, 2023 23:46
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