Skip to content

Inconsistent feature selection among distributed binaries #1829

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

Closed
shadowsocks69420 opened this issue Dec 31, 2024 · 4 comments
Closed

Inconsistent feature selection among distributed binaries #1829

shadowsocks69420 opened this issue Dec 31, 2024 · 4 comments

Comments

@shadowsocks69420
Copy link

shadowsocks69420 commented Dec 31, 2024

The default features enabled in different distribution channels are inconsistent which may cause confusion to the end user. For example:

  • Binaries distributed through GitHub releases, Docker images, were built with full feature enabled.
  • However, binaries distributed via cargo install, brew install (which inherits cargo install) use the default feature only.

This confused me because the online config delivery (SIP008) works in my docker container but not on my MacBook because the two use different feature sets.

I suggest that perhaps we should make the full feature the default, so that end users are free from confusion caused by these kinds of inconsistencies. With full-extra being introduced, the full feature isn't reflecting the "fullness" of the features accurately anymore anyway. With reasonably more features enabled by default, this also saves power users from compiling and packaging the binaries themselves when functionalities like SIP008 are needed, which admittedly, is a significant amount of work we would like to delegate :)

As always, thanks for your hard work on this project! Would love to hear your thoughts on this.

@zonyitoo
Copy link
Collaborator

Cargo builds are exactly the same as building from sources (github). Users that install from source should know exactly what they are building and what is the output. I am actually going to remove all the default features in the future.

Uses that use this project as a library should expect it only builds the necessary components of this project. full includes many extra features that not many users need, for example, socks4, DoT, DoH, ...

On the other hand, the build script in the project HomeBrew, and also nixpkg, ArchLinux, ... are maintained by many other enthusiasts. I cannot control their build parameters, you should open an issue in their project.

At the last, the prebuilt binaries. These binaries are for users that don't want to build from source and expecting to use most of the features out-of-the-box. full is the out-of-the-box feature set.

@shadowsocks69420
Copy link
Author

shadowsocks69420 commented Dec 31, 2024

Thanks for your quick response!

Users that install from source should know exactly what they are building and what is the output.

This is quite a bold assumption to make. In my opinion, cargo install also differs from "compiling from the source" in a traditional sense. If you don't agree with me on this, then maybe consider moving the cargo install option under the "Build from source" section, or at least document the functional differences between the listed installation options as I'm fairly confident that such inconsistencies are beyond normal user expectations.

Uses that use this project as a library should expect it only builds the necessary components of this project. full includes many extra features that not many users need, for example, socks4, DoT, DoH, ...

Just to clarify, I'm not suggesting changes to the library features. I see this project already split into separate binary and library crates, the suggested feature change is only meant for the binary crates.

I agree with you that a library crate should not provide overly bloated default features, heck, I'd argue that the concept of "default features" itself is inherently doing more harm than good (maintaining a working default while also allowing the freedom of choosing rustls vs native-tls for example). But that opinion only applies from a library consumer standpoint, the story becomes very different when we change perspective to binary consumers.

On the other hand, the build script in the project HomeBrew, and also nixpkg, ArchLinux, ... are maintained by many other enthusiasts. I cannot control their build parameters, you should open an issue in their project.

I understand that these packages may not be officially maintained. But I believe there is definitely room for improvement that can be made in this project to help/guide our package maintainers. And yes, "we cannot control their build parameters", but we can definitely provide a more reasonable default for our binaries, even solely for cargo install, it would also help them too. If I understand this correctly, this should be something that can also be done without affecting any library consumers by only changing the required-features in the [[bin]] section.

At the last, the prebuilt binaries. These binaries are for users that don't want to build from source and expecting to use most of the features out-of-the-box. full is the out-of-the-box feature set.

This is the exact same reason why I filed this issue. Binary consumers mostly expect stuff to work out of the box. Having inconsistencies like this is something that I believe should be fixed/resolved, especially when we are packaging more features by default "officially".

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jan 1, 2025

Actually you have convinced me a lot. But I am still going to argue about what should be the “default” feature.

Theoretically the “default” feature should only contain the default feature set of shadowsocks, which should be “basic”. But in this project, prebuilt releases are built with “full”.

The current “default” features are there just for compatibility reasons. It’s ok to be changed to any reasonable feature set.

@shadowsocks69420
Copy link
Author

shadowsocks69420 commented Jan 2, 2025

Actually you have convinced me a lot.

Glad to hear that. Thank you for being open-minded.

But I am still going to argue about what should be the “default” feature.

Theoretically the “default” feature should only contain the default feature set of shadowsocks, which should be “basic”. But in this project, prebuilt releases are built with “full”.

The current “default” features are there just for compatibility reasons. It’s ok to be changed to any reasonable feature set.

There are definitely many arguments that can be made on whether a specific feature should be included by default or not.

However, as a starting point, I suggest we first establish the parity of feature selection between the default of cargo install and what has been offered in the official binaries.
If there are significant drawbacks to some features being offered by default and we need to change them in the future, I think the users would greatly appreciate a major version bump to help them pay more attention to such changes.

In terms of the general direction of default feature selection for binaries - my opinion is that - we should offer binaries that are "battery-included" to save users from compiling/packaging the software themselves when they are not comfortable with doing so. This also has the added benefit of discouraging our users from downloading binaries from untrusted parties / risky sources.

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

No branches or pull requests

2 participants