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

chore: Remove self-update from default crate features #1139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alerque
Copy link

@alerque alerque commented Apr 26, 2024

The self-updater feature is problematic for distro package builders where the executable does not have permission and should not be trying to monkey with the system installed binaries. Distros have their own update mechanisms that shouldn't be tampered with by every app that comes along. This means distro have to build with --no-default-features for apps that include self-updaters by default, but that also means we have to maintain a list of features we do want. This is not only tedious it is error-prone because there is a very good chance of new features getting overlooked when doing version bumps.

Setting up a feature group like this makes it much easier for distros to build without the unwanted features without getting out of sync with upstream enhancements over time.

@alerque
Copy link
Author

alerque commented Apr 26, 2024

Personally I would actually recommend taking the self updater out of the default feature list entirely. Folks can and should get their updates by whatever means they first used to get it (distro packages, cargo install, source builds, etc.). The only exception I would make is for the upstream built binaries attached to releases. Those probably can and should have the feature enabled. That means adding the feature manually to the CI builds, but setting it to be off by default in everything else. I made this PR to be the least intrusive change possible, but if you would accept the change to the default feature list I would be happy to adapt it for this recommendation.

@simonsan
Copy link
Contributor

Personally, I'm up for adapting default features in the way, that you can package with default-features instead of using another newly introduced feature. I'm myself not a very big fan of having self-updatable executables. So I understand where you are coming from. Let's wait for @aawsome opinion though, and if he agrees we can use the mentioned changes to the default features, instead.

I can update our CI/CD workflows then.

@aawsome
Copy link
Member

aawsome commented Apr 26, 2024

I fully agree with @simonsan

@alerque
Copy link
Author

alerque commented Apr 26, 2024

Thanks for the review and being willing to re-consider the defaults. I'll update my changes with that in mind.

The self-updater feature is problematic for many different distribution
channels including distro packages, direct `cargo install`, various
package managers and containers, etc. It will even goof up folks
installing from source builds they made themselves. Either the binary
does not have permission and should not be trying to monkey with the
system installed binaries or the rest of the things besides the binary
will end up out of sync ith packaging.

Distros have their own update mechanisms that shouldn't be tampered with
by every app that comes along. This means distro have to build with
--no-default-features for apps that include self-updaters by default,
but that also means we have to maintain a list of features we do want.
This is not only tedious it is error-prone because there is a very good
chance of new features getting overlooked when doing version bumps.

Additionally many other install workflows should be updating using those
workflows, not being tampered with internally this way.

The binaries posted on releases that people might download and place
manually are good candidates for enabling this feature.
@alerque
Copy link
Author

alerque commented Apr 26, 2024

I've replaced 5c1089d with 8be7b74 which just drops the self-updater from the default flag list.

I had a look at CI but it isn't apparent to me what ends up where. None of the RPM or other package builds should have this feature enabled (as the meta data for the package would be out of sync even if somehow the binary got updated). On the other hand any binaries that the end use would download and place manually themselves are candidates for having this re-enabled.

@simonsan Feel free to tag team on this PR or point me in the right direction or whatever...

@aawsome aawsome changed the title chore: Group all features without self-updater for simplified distro packaging chore: Remove self-update from default crate features Apr 26, 2024
Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

LGTM

@simonsan
Copy link
Contributor

This needs to wait a bit with merging though, as I need to update the CD first to build rustic-nightly and release with the corresponding added features. Will merge it when ready.

@simonsan simonsan added C-enhancement Category: New feature or request A-packaging Area: related to packaging S-blocked Status: Blocked from merging/working on due to some issue A-build Area: Related to the build system labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build Area: Related to the build system A-packaging Area: related to packaging C-enhancement Category: New feature or request S-blocked Status: Blocked from merging/working on due to some issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants