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

Remove toolchain --install/--uninstall/--install-dir #1614

Merged
merged 5 commits into from Mar 7, 2024

Conversation

mosteo
Copy link
Member

@mosteo mosteo commented Mar 5, 2024

Fixes #1612

@mosteo mosteo marked this pull request as ready for review March 5, 2024 21:49
Copy link
Member

@Fabien-Chouteau Fabien-Chouteau left a comment

Choose a reason for hiding this comment

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

So I worked on the same thing in parallel 🤦‍♂️

In my commit I renamed alr toolchain to alr default-toolchain because I think it will remove some confusion on how Alire handles toolchains.

@@ -1,44 +1,53 @@
"""
Check folders when installing binary compiler crates at a custom location
through `alr get`
Copy link
Member

Choose a reason for hiding this comment

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

Why not alr install --prefix here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just because it is the closest to what the test was doing. We could add a new test that checks a similar idea but with alr install.

Copy link
Member

Choose a reason for hiding this comment

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

alr toolchain --install was introduced for people that wanted to get a toolchain from the Alire index but not use it in the Alire context.

I feel like alr install is a better option for this use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll adapt the test.


Toolchain installation for use by Alire is still possible using
`alr toolchain --select`. For the installation of toolchains outside of Alire
management, `alr get` covers the `alr toolchain --install --install-dir` use
Copy link
Member

Choose a reason for hiding this comment

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

Same as below, why not recommend alr install --prefix here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, because for people that was using --install-dir, this is what they would find more similar. But if we want people to just use alr install, we can omit alr get altogether.

@mosteo
Copy link
Member Author

mosteo commented Mar 6, 2024

So I worked on the same thing in parallel 🤦‍♂️

:-(

In my commit I renamed alr toolchain to alr default-toolchain because I think it will remove some confusion on how Alire handles toolchains.

This will break a lot of workflows/actions out there, ours included... Do you think there's that much risk of confusion, given the --select?

@Fabien-Chouteau
Copy link
Member

In my commit I renamed alr toolchain to alr default-toolchain because I think it will remove some confusion on how Alire handles toolchains.

This will break a lot of workflows/actions out there, ours included...

That's why we are doing a major release ^^

Do you think there's that much risk of confusion, given the --select?

I think we do see misconceptions from time to time.

Which makes me thing we could also take this opportunity to rename alr config into alr settings (see #1607 ).

@mosteo
Copy link
Member Author

mosteo commented Mar 6, 2024

In my commit I renamed alr toolchain to alr default-toolchain because I think it will remove some confusion on how Alire handles toolchains.

This will break a lot of workflows/actions out there, ours included...

That's why we are doing a major release ^^

I feel this is a point where I must resist, and not so much for the brokenness but because I find default-toolchain unnecessarily long, a bit ugly, and itself a bit misleading as for example when several are installed, the output shows both default and non-default toolchains:

$ alr toolchain
CRATE         VERSION       STATUS    NOTES                                     
gnat_native   11.2.4        Available                                           
gnat_native   12.2.1        Available                                           
gnat_native   13.2.1        Default

I'd be less objecting to alr toolchain --select-default or even alr toolchain --set-default.

But I have no more arguments so if you're truly convinced this will be a net gain, I won't object further.

Which makes me thing we could also take this opportunity to rename alr config into alr settings (see #1607 ).

Sounds good, I saw the exchange with Maxim.

I'm a bit uneasy about introducing such breaking changes so late in the RC period, in particular if we dont have a 2nd RC (which I didn't plan to have). What do you think of having the old name as an undocumented alias that emits a warning pointing to the new command?

@mosteo
Copy link
Member Author

mosteo commented Mar 6, 2024

The last push completes the removal. Any command renamings I would do in a separate PR.

@mosteo mosteo merged commit 6238ab1 into alire-project:master Mar 7, 2024
28 checks passed
@mosteo mosteo deleted the fix/remove-install-dir branch March 7, 2024 11:21
@Fabien-Chouteau
Copy link
Member

@mosteo I started working on the alr settings modification.

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.

alr toolchain --install / --install-dir now obsolete
2 participants