Consider remove 'asm' feature or offer an option to disable it

[KyleRicardo]

Ref RustCrypto/asm-hashes/issues#41
According to this benchmark, the all-intrinsics impl of the crypto has almost the same performance as the asm impl. (Or even slightly better?)

Ref RustCrypto/asm-hashes/issues#17
So the author said:

It seems like intrinsics can provide mostly equivalent performance but in a much more portable way.

Considering that repo has no new commit since almost a year ago, I presume it is now in passive maintainance mode. The 'asm' feature is actually 'deprecated'. It exists because of backwards compatibility.

A pure Rust approach solves a lot of problems, especially relating to portability.

MSVC build is completely broken and this is very frustrating because it is the official prompt in rustup-init.exe. Installing GNU on windows is upsetting.
IMO, the 'asm' feature in deps can be removed. Or an option can be offered to this lib so users can choose.

[zonyitoo]

shadowsocks-crypto/Cargo.toml

Lines 52 to 57 in 22e74ea

	#[target.'cfg(all(windows, any(target_arch = "x86", target_arch = "x86_64"), target_env = "gnu"))'.dependencies]
	#md-5 = { version = "0.10", features = ["asm"] }
	#sha1 = { version = "0.10", features = ["asm"], optional = true }
	#[target.'cfg(all(windows, target_arch = "aarch64", target_env = "gnu"))'.dependencies]
	#sha1 = { version = "0.10", features = ["asm"], optional = true }

These lines for Windows are completely removed, why would you think that MSVC build fail is related to the "asm" feature?

[KyleRicardo]

Sorry, my bad. I ignored the unix constraint before the x86 arch.

shadowsocks-crypto/Cargo.toml

Lines 45 to 50 in 22e74ea

	[target.'cfg(all(unix, any(target_arch = "x86", target_arch = "x86_64")))'.dependencies]
	md-5 = { version = "0.10", features = ["asm"] }
	sha1 = { version = "0.10", features = ["asm"], optional = true }
	[target.'cfg(all(unix, target_arch = "aarch64", any(target_os = "linux", target_os = "macos")))'.dependencies]
	sha1 = { version = "0.10", features = ["asm"], optional = true }

Despite this, I found a very strange problem during my test. Here it is (to reproduct the problem):

Create a new crate, either bin or lib, doesn't matter. I'm gonna call it crypto-bin. Now the directory tree is like this:

crypto-bin
├── Cargo.toml
└── src
    └── main.rs

Leaving alone the main.rs, I added a dep in Cargo.toml. So it is like:

[package]
name = "crypto-bin"
version = "0.1.0"
edition = "2021"

[dependencies]
shadowsocks-crypto = "0.4"

And I immediately ran cargo tree command:

➜ crypto-bin  cargo tree
    Updating crates.io index
crypto-bin v0.1.0 (F:\Project\crypto-bin)
└── shadowsocks-crypto v0.4.0
    ├── aes-gcm v0.9.4
    │   ├── aead v0.4.3
    │   │   └── generic-array v0.14.5        
    │   │       └── typenum v1.15.0
    │   │       [build-dependencies]
    │   │       └── version_check v0.9.4     
    │   ├── aes v0.7.5
    │   │   ├── cfg-if v1.0.0
    │   │   ├── cipher v0.3.0
    │   │   │   └── generic-array v0.14.5 (*)
    │   │   ├── cpufeatures v0.2.2
    │   │   └── opaque-debug v0.3.0
    │   ├── cipher v0.3.0 (*)
    │   ├── ctr v0.8.0
    │   │   └── cipher v0.3.0 (*)
    │   ├── ghash v0.4.4
    │   │   ├── opaque-debug v0.3.0
    │   │   └── polyval v0.5.3
    │   │       ├── cfg-if v1.0.0
    │   │       ├── cpufeatures v0.2.2
    │   │       ├── opaque-debug v0.3.0
    │   │       └── universal-hash v0.4.1
    │   │           ├── generic-array v0.14.5 (*)
    │   │           └── subtle v2.4.1
    │   └── subtle v2.4.1
    ├── cfg-if v1.0.0
    ├── chacha20poly1305 v0.9.0
    │   ├── aead v0.4.3 (*)
    │   ├── chacha20 v0.8.1
    │   │   ├── cfg-if v1.0.0
    │   │   ├── cipher v0.3.0 (*)
    │   │   ├── cpufeatures v0.2.2
    │   │   └── zeroize v1.4.3
    │   ├── cipher v0.3.0 (*)
    │   ├── poly1305 v0.7.2
    │   │   ├── cpufeatures v0.2.2
    │   │   ├── opaque-debug v0.3.0
    │   │   └── universal-hash v0.4.1 (*)
    │   └── zeroize v1.4.3
    ├── hkdf v0.12.3
    │   └── hmac v0.12.1
    │       └── digest v0.10.3
    │           ├── block-buffer v0.10.2
    │           │   └── generic-array v0.14.5 (*)
    │           ├── crypto-common v0.1.3
    │           │   ├── generic-array v0.14.5 (*)
    │           │   └── typenum v1.15.0
    │           └── subtle v2.4.1
    ├── md-5 v0.10.1
    │   └── digest v0.10.3 (*)
    ├── rand v0.8.5
    │   ├── rand_chacha v0.3.1
    │   │   ├── ppv-lite86 v0.2.16
    │   │   └── rand_core v0.6.3
    │   │       └── getrandom v0.2.7
    │   │           └── cfg-if v1.0.0
    │   └── rand_core v0.6.3 (*)
    └── sha1 v0.10.1
        ├── cfg-if v1.0.0
        ├── cpufeatures v0.2.2
        └── digest v0.10.3 (*)

As you can see, sha1 and md5 is normal without the asm dep.

But when I put this crate into a workspace, funny things happened.
I copied the crypto-bin folder into a new folder called crypto-joke, w/ a new Cargo.toml as a workspace manifest in it.
So the structure of directory was like this:

crypto-joke
├── Cargo.toml
└── crypto-bin
    ├── Cargo.toml
    └── src
        └── main.rs

and the outer Cargo.toml had the following content:

[workspace]
members = [
    "crypto-bin",
]
default-members = [
    "crypto-bin",
]

and then I ran cargo tree command again:

➜ crypto-joke  cargo tree
    Updating crates.io index
crypto-bin v0.1.0 (F:\Project\crypto-joke\crypto-bin)
└── shadowsocks-crypto v0.4.0
    ├── aes-gcm v0.9.4
    │   ├── aead v0.4.3
    │   │   └── generic-array v0.14.5        
    │   │       └── typenum v1.15.0
    │   │       [build-dependencies]
    │   │       └── version_check v0.9.4     
    │   ├── aes v0.7.5
    │   │   ├── cfg-if v1.0.0
    │   │   ├── cipher v0.3.0
    │   │   │   └── generic-array v0.14.5 (*)
    │   │   ├── cpufeatures v0.2.2
    │   │   └── opaque-debug v0.3.0
    │   ├── cipher v0.3.0 (*)
    │   ├── ctr v0.8.0
    │   │   └── cipher v0.3.0 (*)
    │   ├── ghash v0.4.4
    │   │   ├── opaque-debug v0.3.0
    │   │   └── polyval v0.5.3
    │   │       ├── cfg-if v1.0.0
    │   │       ├── cpufeatures v0.2.2
    │   │       ├── opaque-debug v0.3.0
    │   │       └── universal-hash v0.4.1
    │   │           ├── generic-array v0.14.5 (*)
    │   │           └── subtle v2.4.1
    │   └── subtle v2.4.1
    ├── cfg-if v1.0.0
    ├── chacha20poly1305 v0.9.0
    │   ├── aead v0.4.3 (*)
    │   ├── chacha20 v0.8.1
    │   │   ├── cfg-if v1.0.0
    │   │   ├── cipher v0.3.0 (*)
    │   │   ├── cpufeatures v0.2.2
    │   │   └── zeroize v1.4.3
    │   ├── cipher v0.3.0 (*)
    │   ├── poly1305 v0.7.2
    │   │   ├── cpufeatures v0.2.2
    │   │   ├── opaque-debug v0.3.0
    │   │   └── universal-hash v0.4.1 (*)
    │   └── zeroize v1.4.3
    ├── hkdf v0.12.3
    │   └── hmac v0.12.1
    │       └── digest v0.10.3
    │           ├── block-buffer v0.10.2
    │           │   └── generic-array v0.14.5 (*)
    │           ├── crypto-common v0.1.3
    │           │   ├── generic-array v0.14.5 (*)
    │           │   └── typenum v1.15.0
    │           └── subtle v2.4.1
    ├── md-5 v0.10.1
    │   ├── digest v0.10.3 (*)
    │   └── md5-asm v0.5.0
    │       [build-dependencies]
    │       └── cc v1.0.73
    ├── rand v0.8.5
    │   ├── rand_chacha v0.3.1
    │   │   ├── ppv-lite86 v0.2.16
    │   │   └── rand_core v0.6.3
    │   │       └── getrandom v0.2.7
    │   │           └── cfg-if v1.0.0
    │   └── rand_core v0.6.3 (*)
    └── sha1 v0.10.1
        ├── cfg-if v1.0.0
        ├── cpufeatures v0.2.2
        ├── digest v0.10.3 (*)
        └── sha1-asm v0.5.1
            [build-dependencies]
            └── cc v1.0.73

Now sha1 had the dependency sha1-asm and md-5 had the dependency md5-asm. NOTHING else changed.

Unbelievable. Did I do something WRONG? Cuz I felt I did nothing.

Sorry to bother you but I got little clue by Googling this. Hope you can help me understand.

Thanks in advance!

[zonyitoo]

I think it would be related to cargo's feature resolver's bug. Use edition = 2021 in all your projects and see if it helps.

https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html

[KyleRicardo]

Ok, thanks, but that's exact what I use in Cargo.toml as I mentioned above:

[package]
name = "crypto-bin"
version = "0.1.0"
edition = "2021"

[dependencies]
shadowsocks-crypto = "0.4"

I made a patch to your lib which completely removed asm related conditional feature according to this guide, turned out to work fine without build failure.

I still hold the opinion that asm featured can be completely removed due to equivalent performance and better portability.

If you don't agree, then maybe we should report this bug to cargo official?

[zonyitoo]

You could make a PR for this.

[KyleRicardo]

Thanks, PR created.