Support cc as an optional alternative to cmake

[Jake-Shadle]

While compiling another project I noticed I was not able to cross compile to x86_64-pc-windows-msvc, due to cmake. We've banned cmake in our projects both because it (generally speaking) complicates/makes impossible cross compilation, as well as just adding another dependency/point of failure/hindrance to contributions.

This is mainly a straight forward port of the cmake script(s) to cc but with one important difference, namely that target features are now respected.

With cmake, zlib-ng is probing the compiler/target headers for intrinsics/features/definitions, and importantly, can enable features irrespective of the target/features supplied to cargo/rustc. For example, cargo build --features zlib-ng --target x86_64-unknown-linux-gnu on my current machine will enable avx2, sse2, sse3, sse4.2, pclmulqdq, xsave, avx512, and vpclmulqdq, despite the only target features enabled by default for that target being fxsr, sse, and sse2.

With the change to cc, I made it so that the target features selected by the user are what are actually respected, instead of generating binaries that won't work on CPUs without 1 or more features that the machine the binary was built on did have.

The one caveat to this is that avx512 target features are not currently stable in rustc, so I've added an additional feature that needs to be enabled for those features to be probed for, but maybe there is a more elegant way to support that in a way that makes sense to the end user.

I also didn't port the powerpc, s390, or riscv related optimization features since this PR is already quite large, but it should be trivial to add support in subsequent PRs.

[Byron]

Thanks so much for tackling this! I agree that less cmake will be better for everyone.

Previously it was my understanding that zlib-ng ships with cmake as build system, and superseding it with an own implementation also means it needs to be maintained here.

For that reason I definitely won't be merging this PR without explicit support from the libs team, and I think it would be easiest to use their channel to probe for additional opinions.

(I also converted the PR into a draft while CI is failing)

[NobodyXu]

With cmake, zlib-ng is probing the compiler/target headers for intrinsics/features/definitions, and importantly, can enable features irrespective of the target/features supplied to cargo/rustc. For example, cargo build --features zlib-ng --target x86_64-unknown-linux-gnu on my current machine will enable avx2, sse2, sse3, sse4.2, pclmulqdq, xsave, avx512, and vpclmulqdq, despite the only target features enabled by default for that target being fxsr, sse, and sse2.

I see, that might be why after upgrading libz-ng to latest release break our cargo-binstall cross compilation on aarch64 linux.

We use cargo-zigbuild for cross compilation and it was working until libz-ng is upgraded rust-cross/cargo-zigbuild#210

[joshtriplett]

With cmake, zlib-ng is probing the compiler/target headers for intrinsics/features/definitions, and importantly, can enable features irrespective of the target/features supplied to cargo/rustc. For example, cargo build --features zlib-ng --target x86_64-unknown-linux-gnu on my current machine will enable avx2, sse2, sse3, sse4.2, pclmulqdq, xsave, avx512, and vpclmulqdq, despite the only target features enabled by default for that target being fxsr, sse, and sse2.

zlib-ng should only be doing that if given WITH_NATIVE_INSTRUCTIONS; otherwise, it should be using runtime detection, which uses these intrinsics but only after checking the CPU capabilities at runtime. If it's using features from the host CPU in any other circumstance, that's absolutely a bug.

[Jake-Shadle]

zlib-ng should only be doing that if given WITH_NATIVE_INSTRUCTIONS; otherwise, it should be using runtime detection, which uses these intrinsics but only after checking the CPU capabilities at runtime. If it's using features from the host CPU in any other circumstance, that's absolutely a bug.

Yes, I was wrong about this, the probing for intrinsics just adds the source files and appropriate compiler flags if the compiler supports them, the runtime detection will only use them if the host CPU supports them.

I think if this PR would otherwise be accepted I can change the code to just do what zlib-ng does and enable all the source files/flags supported by the C compiler rather than relying on the target features to be enabled in rustc, but I'm not going to spend more time on this PR unless it's indicated it would be merged barring that change.

[Jake-Shadle]

Is there any chance of getting a review on this? I'd rather close this than have it sitting.

[NobodyXu]

@Jake-Shadle I'd recommend you to open a new topic in zulip channel https://rust-lang.zulipchat.com/#narrow/stream/351149-t-libs.2Fcrates and cc those who works in libz-ng

[wcampbell0x2a]

This would be amazing, as I would love to use this library for https://github.com/wcampbell0x2a/backhand/pulls, but don't want cmake in my build system.

[joshtriplett]

I would love to see this happen, but also, this seems like a massive increase in complexity and maintenance burden for this crate, to duplicate the build system of upstream.

Is there any chance we could get upstream zlib-ng to add support for a non-cmake build system and then use that, so that we don't have to effectively rewrite their build system downstream and then maintain that rewrite as upstream's build system continues to change in the future?

[Jake-Shadle]

Other than the *.in file transformations, all of the complexity in the build script beyond what a normal project has is simply due to the compiler probing for features and headers. IMO this is incredibly over conservative in an attempt to support really old compilers or "esoteric" targets that are already supported by vanilla zlib.

The easiest way to reduce complexity would just be to remove all of the probing and just....turn on everything for the target. If the user is using an extremely old compiler or targeting something that doesn't support all of the features, they can either 1. special case the build script to remove the specific feature(s) not supported by their concrete use-case 2. disable the zlib-ng feature to just use vanilla zlib instead. The build script could also just detect the failure and fallback to zlib automatically and print a warning about it, but generally I find a hard error to let the user know something went wrong is better so that they can either fix it or workaround it.

Is there any chance we could get upstream zlib-ng to add support for a non-cmake build system and then use that, so that we don't have to effectively rewrite their build system downstream and then maintain that rewrite as upstream's build system continues to change in the future?

What would that be though? I don't see what reason they would have to support another one for a single downstream project.

[NobodyXu]

If the user is using an extremely old compiler or targeting something that doesn't support all of the features

IMO adding environment variable for these features make more sense

[joshtriplett]

all of the complexity in the build script beyond what a normal project has is simply due to the compiler probing for features and headers

That's not the only complexity or maintenance burden here. There's also the list of files (needs updating every time zlib-ng adds a new source file), and the list of symbols to define (needs updating every time zlib-ng's build system adds a new such symbol).

I'm not saying we shouldn't consider doing this. I'm observing that if we do, we change the maintenance burden from "upgrade to new upstream version of zlib-ng and make sure tests pass" to "upgrade to new upstream version of zlib-ng, audit the build system to see if they've added any new symbols, and port any logic needed to set those symbols".

The easiest way to reduce complexity would just be to remove all of the probing and just....turn on everything for the target.

Defaulting to building zlib-ng in "probe for everything you know about at runtime" mode does seem like the right answer, particularly if it simplifies the build logic; if people want to hard-enable or hard-disable any features, or probe at compile time, perhaps we could control the use of cmake versus the non-cmake simplified build system via feature flags.

That said, that'd be a fallback plan if upstream isn't willing to consider a non-cmake build system.

I don't see what reason they would have to support another one for a single downstream project.

The same reason this project would consider doing so: to try to help their users who don't want to add a cmake dependency, at the expense of taking on additional complexity. The difference is that zlib-ng is better positioned to manage that complexity than we are.

I know at least one project that's considering a build system rewrite away from cmake for the exact same reason: aws-lc.

[Jake-Shadle]

That's not the only complexity or maintenance burden here. There's also the list of files (needs updating every time zlib-ng adds a new source file), and the list of symbols to define (needs updating every time zlib-ng's build system adds a new such symbol).

I'm not saying we shouldn't consider doing this. I'm observing that if we do, we change the maintenance burden from "upgrade to new upstream version of zlib-ng and make sure tests pass" to "upgrade to new upstream version of zlib-ng, audit the build system to see if they've added any new symbols, and port any logic needed to set those symbols".

That's the normal amount of burden that any cc-based build script needs to do, looking at zlib-ng history there does seem to be more churn wrt build system affecting file/etc changes compared to the typical C project, but not to a degree that would make version upgrades take a significant longer period of time barring large version updates or a too long period of not updating.

Defaulting to building zlib-ng in "probe for everything you know about at runtime" mode does seem like the right answer, particularly if it simplifies the build logic; if people want to hard-enable or hard-disable any features, or probe at compile time, perhaps we could control the use of cmake versus the non-cmake simplified build system via feature flags.

I think that's a reasonable compromise, that way if the simpler cc-based build fails the error can tell them to turn on the cmake feature to do the probing to support their limited environment/target but most won't need to do that.

The same reason this project would consider doing so: to try to help their users who don't want to add a cmake dependency, at the expense of taking on additional complexity. The difference is that zlib-ng is better positioned to manage that complexity than we are.

They support regular make, presumably for those who don't want to use cmake, I just am not aware of any other build systems in the C space that don't add external dependencies (python, lua, etc) or are platform specific (bash scripts, vcxproj etc) that one would have any hope of proposing to them.

[Byron]

Defaulting to building zlib-ng in "probe for everything you know about at runtime" mode does seem like the right answer, particularly if it simplifies the build logic; if people want to hard-enable or hard-disable any features, or probe at compile time, perhaps we could control the use of cmake versus the non-cmake simplified build system via feature flags.

I think that's a reasonable compromise, that way if the simpler cc-based build fails the error can tell them to turn on the cmake feature to do the probing to support their limited environment/target but most won't need to do that.

The above part of the conversation gave me an idea I want to share, right after sharing my disposition about this: I feel strongly about not wanting to chase zlib-ng build system changes to the point where I wont hit the merge button for that reason alone. However, I also wouldn't oppose to somebody else doing it.

With this basis, something that would work under such circumstances is the addition of a cargo feature that uses this build system instead of the default one. I would even mark it with (something like) use-alternative-build-system-alpha to indicate it's early and unstable and may not even be working unless it's kept working by the people who are using it.

That way the maintenance burden is shifted to contributors and there are no guarantees that it keeps working, which allows us to learn what it means to have it and maybe maintain it. Something I can imagine will happen is that as the build system gets patched to support upstream changes, I see and learn how to do that, maybe realising that it is indeed very doable which would ease the transition to being maintained here as well. Something that could also happen is the 'devil in the detail' which shows it's much less feasible, leading to its removal. But that way we could at least find out as this feature is without any doubt valuable and should be considered.

[NobodyXu]

They support regular make, presumably for those who don't want to use cmake

Would it makes sense to implement a command that translate make to cc-rs rust code?

It can be run whenever zlib-ng version is bumped in CI.

[Jake-Shadle]

I've now split it so that cmake and cc are supported in parallel, cmake with the regular zlib-ng feature and cc with zlib-ng-no-cmake that way all of the crates that currently use zlib-ng will continue working as before, but crates that don't want to use cmake can expose a feature flag to use the non-cmake feature instead.

The code is much more simple now and no longer does intrinsic/header detection, and rather just turns on all reasonable features. Amusingly, this means that several linux-gnu targets are skipped when building without cmake in CI due to those targets using an ancient (almost 8 years old) version of GCC that doesn't support some AVX-512 flags.

[Jake-Shadle]

I'm going to close this since it doesn't appear it will be merged, I'll leave the fork up for now in case someone wants to carry it forward, or just use it for reference or something.

[ofek]

If you still have time I think it would be good to click open once more because I think there is desire for this to be merged but the rebase happened only a few days ago to resolve the conflicts. I and a few others I know of have a great need for this so it would be nice to give the maintainers a little bit more time to review, please 🙏

[Jake-Shadle]

It's been up for 4 months with no progress and no response to https://rust-lang.zulipchat.com/#narrow/stream/351149-t-libs.2Fcrates/topic/PR.20review.20request/near/418653205, I think it's time for someone else to drive this forward if they care about it.

[ofek]

I understand your perspective, thanks. It's a bummer that so many folks will not use ng/the best implementation because of CMake. I don't have time unfortunately or else I would do this myself.

[Byron]

@Jake-Shadle I hope it's not too late to prevent forking or other efforts going into this. From what I recall, we have an agreement on how to handle and maintain this CC-based version of it.

Since 4 months passed for interested maintainers to show up and double-check my sanity, I think it's fair to say there was enough time to respond. Thus, I am happy to take it upon myself to go forward with this and try, hoping there will be a lot of benefit for the community.

Since you closed it, I wouldn't want to merge unless I get your permission.

[Jake-Shadle]

I'd be happy to have this merged, was just closing PRs and issues in my backlog that didn't seem to be going anywhere.

[Byron]

Great to hear! If a release is desired, I'd need a PR with the version change as I don't think I can merge my own PRs.

[ofek]

Is there no way to use this new option with cross? ofek/pyapp@6a98598 (#137)

I'm guessing the images cross uses have very old compiler versions?