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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Sep 10, 2024
Merged

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

merged 2 commits into from
Sep 10, 2024

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Apr 26, 2024 •
edited
Loading

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
Contributor 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.

@alerque alerque mentioned this pull request Apr 26, 2024
Merged
@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
Contributor 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.

@alerque
chore: Remove self-updater from default features
8be7b74 
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
Contributor 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
aawsome
aawsome approved these changes Apr 26, 2024
View reviewed changes
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
@simonsan simonsan self-assigned this Sep 10, 2024
@simonsan simonsan added this pull request to the merge queue Sep 10, 2024
@simonsan simonsan removed the S-blocked Status: Blocked from merging/working on due to some issue label Sep 10, 2024
Merged via the queue into rustic-rs:main with commit ab13071 Sep 10, 2024
25 checks passed
@simonsan
Copy link
Contributor

Thanks ;)

@rustic-release-plz rustic-release-plz bot mentioned this pull request Sep 24, 2024
Merged
simonsan pushed a commit that referenced this pull request Sep 29, 2024
@rustic-release-plz @aawsome
chore: release v0.9.0 (#1256)
ee3f099 
## 🤖 New release
* `rustic-rs`: 0.8.1 -> 0.9.0 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.9.0](v0.8.1...v0.9.0)
- 2024-09-29

### Added

- *(commands)* Add list indexpacks and list indexcontent commands
([#1254](#1254))
- *(commands)* Add option `--only-identical` for `diff` to allow for
bitrot check ([#1250](#1250))
- *(commands)* ls: Add option --json
([#1251](#1251))
- *(commands)* [**breaking**] copy: Use config profile as target
([#1131](#1131))
- *(commands)* backup: Add option --long
([#1159](#1159))

### Fixed

- *(deps)* update rust crate libc to v0.2.159
([#1257](#1257))
- *(config)* [**breaking**] use multiple options only as array in config
profile ([#1240](#1240))
- *(interactive)* Allow snapshots to be modified and marked to forget
([#1253](#1253))
- make ls and find show the year of mtime date
([#1249](#1249))
- ls: Remove printing trailing space
([#1247](#1247))
- webdav/forget: correctly use application config
([#1241](#1241))

### Other

- update installation instructions in readme to use `--locked` flag for
install from crates.io
- *(deps)* lock file maintenance
([#1269](#1269))
- delete unused xtask pattern directory
- *(deps)* update rust crate rstest to 0.23
([#1267](#1267))
- *(deps)* update rust crate tempfile to v3.13.0
([#1266](#1266))
- *(deps)* update marcoieni/release-plz-action digest to 8b0f89a
([#1265](#1265))
- *(deps)* update embarkstudios/cargo-deny-action action to v2
([#1259](#1259))
- *(deps)* update rustsec/audit-check action to v2
([#1260](#1260))
- *(deps)* update softprops/action-gh-release action to v2
([#1258](#1258))
- *(deps)* update embarkstudios/cargo-deny-action digest to 3f4a782
([#1228](#1228))
- don't let release-plz create GH releases
- exclude the CHANGELOG from dprint formatting
- remove `-dev` description from version for release-plz to work
- remove release-pr workflow and replace with release-plz
- *(cd)* try fixing nightly release pipeline
- *(deps)* Update to new releases
([#1255](#1255))
- Reduce memory usage of restore
([#1069](#1069))
- Update to newest rustic_core
([#1248](#1248))
- update RepositoryErrorKind rustdoc following rustic_core change
([#1237](#1237))
- set development version
- add flag for building with self-update feature for nightly and CD
- Remove self-update from default crate features
([#1139](#1139))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: rustic-release-plz[bot] <182542030+rustic-release-plz[bot]@users.noreply.github.com>
Co-authored-by: Alexander Weiss <[email protected]>
simonsan pushed a commit that referenced this pull request Oct 2, 2024
@rustic-release-plz @aawsome @simonsan
chore: release v0.9.0 (#1256)
1ad9ee9 
## 🤖 New release
* `rustic-rs`: 0.8.1 -> 0.9.0 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.9.0](v0.8.1...v0.9.0)
- 2024-09-29

### Added

- *(commands)* Add list indexpacks and list indexcontent commands
([#1254](#1254))
- *(commands)* Add option `--only-identical` for `diff` to allow for
bitrot check ([#1250](#1250))
- *(commands)* ls: Add option --json
([#1251](#1251))
- *(commands)* [**breaking**] copy: Use config profile as target
([#1131](#1131))
- *(commands)* backup: Add option --long
([#1159](#1159))

### Fixed

- *(deps)* update rust crate libc to v0.2.159
([#1257](#1257))
- *(config)* [**breaking**] use multiple options only as array in config
profile ([#1240](#1240))
- *(interactive)* Allow snapshots to be modified and marked to forget
([#1253](#1253))
- make ls and find show the year of mtime date
([#1249](#1249))
- ls: Remove printing trailing space
([#1247](#1247))
- webdav/forget: correctly use application config
([#1241](#1241))

### Other

- update installation instructions in readme to use `--locked` flag for
install from crates.io
- *(deps)* lock file maintenance
([#1269](#1269))
- delete unused xtask pattern directory
- *(deps)* update rust crate rstest to 0.23
([#1267](#1267))
- *(deps)* update rust crate tempfile to v3.13.0
([#1266](#1266))
- *(deps)* update marcoieni/release-plz-action digest to 8b0f89a
([#1265](#1265))
- *(deps)* update embarkstudios/cargo-deny-action action to v2
([#1259](#1259))
- *(deps)* update rustsec/audit-check action to v2
([#1260](#1260))
- *(deps)* update softprops/action-gh-release action to v2
([#1258](#1258))
- *(deps)* update embarkstudios/cargo-deny-action digest to 3f4a782
([#1228](#1228))
- don't let release-plz create GH releases
- exclude the CHANGELOG from dprint formatting
- remove `-dev` description from version for release-plz to work
- remove release-pr workflow and replace with release-plz
- *(cd)* try fixing nightly release pipeline
- *(deps)* Update to new releases
([#1255](#1255))
- Reduce memory usage of restore
([#1069](#1069))
- Update to newest rustic_core
([#1248](#1248))
- update RepositoryErrorKind rustdoc following rustic_core change
([#1237](#1237))
- set development version
- add flag for building with self-update feature for nightly and CD
- Remove self-update from default crate features
([#1139](#1139))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: rustic-release-plz[bot] <182542030+rustic-release-plz[bot]@users.noreply.github.com>
Co-authored-by: Alexander Weiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aawsome aawsome aawsome approved these changes

Assignees

@simonsan simonsan

Labels
A-build Area: Related to the build system A-packaging Area: related to packaging C-enhancement Category: New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alerque @simonsan @aawsome