nabijaczleweli / cargo-update

A cargo subcommand for checking and applying updates to installed executables
MIT License
1.22k stars 42 forks source link

Rename `cargo-install-update-config` to `...-configure` #132

Closed bosr closed 2 years ago

bosr commented 4 years ago

Hi, thanks for this great cargo command!

One thing bugs me, the installed executables being in the path, the name cargo-install-update-config can be picked as a pkg-config. Some sanity checkers like brew doctor pick this everytime.

Would you consider renaming it to cargo-install-update-configure or something alike? I don't think it would break anything as we don't call it directly.

Thanks in advance!

nabijaczleweli commented 4 years ago

First: thanks!

Otherwise: I tried to find a manpage or docs regarding brew doctor but all i got was products and What does "keg-only" mean? It means the formula is installed only into the cellar which put me to sleep immediately (as I hope you understand). Perhaps you have a link to some documentation?

cargo-install-update-config's interface is stable since it first appeared in v0.8.0 (25 Mar 2017), cargo-install-update's since v0.1.0 (29 Oct 2016) (I think(?), the major releases are just for the library API); I do not want to break them if at all possible.

Also I googled around for "cargo-install-update-config" and there weren't many results (good) but there were a few from someone's CI logs (bad). Not a deal-breaker, but still.

This'd be easier to do if Cargo had any binary package management infrastructure (apt-listchanges springs to mind), but then this crate wouldn't be necessary if that were the case, so.

The only way in which the interface isn't broken globally is by only adding cargo-install-update-configure. This doesn't stop linters from complaining, though, without something like dpkg --path-exclude in /etc/dpkg.cfg[.d], but that's very crude and IIRC from http will build the binary twice, which is not great.

Something like dpkg-divert springs to mind as well, to make this a piece of explicit per-user/site configuration, but, again, Cargo.

hmm, tough nut

nabijaczleweli commented 4 years ago

ah fuck, i just realised i could implement an equivalent to dpkg-divert and hook it into the configure architecture we have already, something like

cargo install-update-config --divert cargo-install-update-config cargo-install-update-configure cargo-update

cargo install-update cargo-update         # if there's an update, or
cargo install-update-divert cargo-update  # to commit manually

Given the right combination of rename()s and updating the values in ~/.cargo/.crates.toml, this should fool cargo into handling this properly, for any package.

Emphasis on the should; there's a .crates2.json now and I'm not sure about the format or who cargo trusts more (the answer to that is probably "piss off", as it has been in the past when it came to any interface to the first-party cargo subcommands and their on-disk state).

It looks like cargo-install already has

--bin <NAME>...   Install only the specified binary

so having a diversion there might not be that far off. Hooking a configuration option onto that would be less fragile.

At any rate, I can't really look at an implementation of either of these right now since my stack is currently full, I'll see again in a few days once i finish some of it at least (sorry!).

nabijaczleweli commented 4 years ago

Tracking: https://github.com/rust-lang/cargo/issues/8250

bosr commented 4 years ago

Hi, thanks for the kind and detailed reply, much appreciated!

I wish I could help you with that cargo workaround, but you are way more skilled than me. Very nice to see you already a lead. It's nothing urgent.

Cheers!

Mellbourn commented 3 years ago

I hit this problem today. I run brew doctor regularly in scripts and they intentionally stop if brew doctor fails, which it does for cargo-install-update-config:

Warning: "config" scripts exist outside your system or Homebrew directories.
`./configure` scripts often look for *-config scripts to determine if
software packages are installed, and which additional flags to use when
compiling and linking.

Having additional scripts in your path can confuse software installed via
Homebrew if the config script overrides a system or Homebrew-provided
script of the same name. We found the following "config" scripts:
  /Users/klas.mellbourn/.cargo/bin/cargo-install-update-config

I can't think of a trivial work-around, since I definitely want to notice other brew doctor problems (they are not uncommon).

It's been almost a year since the previous message on this issue. Is there still a plan to improve this?

Thanks for a great tool!

Mellbourn commented 3 years ago

I did think of a trivial workaround: I manually renamed cargo-install-update-config to cargo-install-update-configure.

It seems to work. Does it break anything? E.g. will install-update ever try to call install-update-config?

nabijaczleweli commented 3 years ago

Mach-O executables aren't scripts, so I haven't a clue so as to what the brew doctor's (whoever they are) problem is, but, yes, it'll be perfectly fine if you just move it.

As for adding a diversion equivalent, I'd rather not, this is much better suited for the package manager (or Cargo, for lack of one), plus the benefit over effort, plus problems, plus scope, plus &c. is decidedly not remotely favourable for me. The current Maufacturer-Suggested Retail Fix is "don't police the executable namespace", especially if it's to work around potential broken autoconf scripts from thirty years ago.

Mellbourn commented 3 years ago

Ok. I understand. To clarify: brew doctor is the sanity checker for macOS homebrew https://docs.brew.sh/Troubleshooting

Thanks for answering, and for all your work with this tool!

ronligt commented 2 years ago

@Mellbourn's solution renaming to cargo-install-update-configure works like a charm and solves the warning produced by brew doctor on macOS.

bosr commented 2 years ago

I'll close this issue as won't fix. The workaround is sufficient, provided that on each update of cargo-update one remembers to rename cargo-install-update-config as cargo-install-update-configure.

k4rtik commented 2 years ago

I don't understand if the workaround is sufficient and does not affect the usability of cargo-update, then what's the harm in implementing it?

When you say "one" you seem to be referring to a minority of users who end up finding this issue. For most others, it's always going to be surprising leading to unnecessary frustration and waste of time searching for a solution. With a simple change here, you have the ability to prevent hundreds of cumulative hours of productivity loss on people.

Or is this issue closed in favor of https://github.com/rust-lang/cargo/issues/8250 ?

nabijaczleweli commented 2 years ago

If the "work-around" is "breaking the API", then that. I know macintosh users are used to their system exploding on each update, but not all of us are resigned to that. If the "work-around" is "distributing both" then experience tells me that's also ass (and would also require manual intervention at least once). The correct solution is for the brew doctor to not police the executable namespace (or to install everything via a brew-doctor-approved method which presumably has a relevant patch). The "correct" work-around is diversions, manual if cargo doesn't do them.

bosr commented 2 years ago

I know macintosh users are used to their system exploding on each update, but not all of us are resigned to that.

I've been using Debian unstable as my main Linux workstation for the last 25 years -> I'd take your statement about macOS stability with quite a bit of salt ;)

@k4rtik, I closed the issue as Won't Fix because I had the impression that @nabijaczleweli won't solve this issue by renaming the binary (he has solid arguments), and not because I disregard macOS users (quite the contrary, I opened the issue). @nabijaczleweli suggests an elegant solution https://github.com/rust-lang/cargo/issues/8250 (thanks!), I hope it gets implemented, and we should probably push for it there, don't you think?

The correct solution is for the brew doctor to not police the executable namespace.

Maybe. This issue https://github.com/Homebrew/legacy-homebrew/issues/18552 explains the warning should be treated as debugging info. I don't think we can convince Homebrew to drop the check entirely because the rationale behind this warning is quite smart and realistic (no one wants to accidentally mess with complex ./configure scripts). I even think it probably applies to all Unixes. With the divert approach, brew doctor will continue checking the safety of macOS users' path.

luckman212 commented 2 years ago

I hit this issue today. I know this issue is closed but add me to the list of people who would like to see the proposed change (rename cargo-install-update-config to cargo-install-update-configure) implemented.

My poor man's workaround for now is to add this stanza to my update script:

if [ -e ~/.cargo/bin/cargo-install-update-config ]; then
  mv -f ~/.cargo/bin/cargo-install-update-config{,ure}
fi
graelo commented 2 years ago

Count me in too!