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

max-pure feature is not pure (pulls in libsqlite3-sys) #1681

Closed
tanriol opened this issue Nov 16, 2024 · 7 comments · Fixed by #1684
Closed

max-pure feature is not pure (pulls in libsqlite3-sys) #1681

tanriol opened this issue Nov 16, 2024 · 7 comments · Fixed by #1684
Labels
acknowledged an issue is accepted as shortcoming to be fixed blocked Issue can't progress due to external dependencies help wanted Extra attention is needed

Comments

@tanriol
Copy link

tanriol commented Nov 16, 2024

Current behavior 😯

$ cargo build --target aarch64-unknown-linux-musl --release --no-default-features --features=max-pure
[...]
error: failed to run custom build command for `libsqlite3-sys v0.30.1`

Expected behavior 🤔

max-pure feature should be actually pure (ideally, CI should ensure it works without a C compiler available at all)

Git behavior

No response

Steps to reproduce 🕹

No response

EliahKagan added a commit to EliahKagan/gitoxide that referenced this issue Nov 17, 2024
This adds more checks to the `pure-rust-build` CI job to reveal
what dependencies of `max-pure` are building C code. The new checks
are failing. They relate to GitoxideLabs#1681 and should pass once that bug is
fixed. The main goal is to produce information helpful for GitoxideLabs#1681.

As currently written, the new steps do not fail the job, because
the failing steps have `continue-on-error: true`. This is so that
regressions (that would fail the preexisting steps) remain readily
detected. This should ideally be temporary; the new steps, if kept,
should eventually be strengthened so they can fail the job.

Currently this includes both regular and build dependencies.

There are two new checks:

1. Search the `max-pure` dependency tree for packages that require
   a C or C++ compiler, or that are in practice only likely to be
   used to build C or C++ code. If any are found, display the whole
   tree, with matching lines highlighted, and fail the step.

2. After all steps that need it to be in working order, break GCC
   in the container by removing the `cc1` binary, then attempt a
   clean `max-pure` build, to reveal the first failure, if any, and
   let it fail the step.

   The `gcc` command itself is needed, because `rustc` calls the
   linker through it, even when no non-Rust code is compiled as
   part of the build.

   As discussed in GitoxideLabs#1664 comments, installing GCC in a way that is
   not broken but omits the ability to compile even C code, while
   it may be possible, is not commonly done and may require long
   running build steps or a new Docker image.

   Fortunately, the `gcc` command is just a "compiler driver"; the
   C compiler in GCC is `cc1`, and this can be removed without
   breaking most uses of GCC that don't involve actual compilation.

   This likewise removes `cc1plus` if found, even though it may not
   be present since we already verified that `g++` is not
   installed. (The deletion of `cc1` is the important part.)

Since the job now cleans and starts a build that fails due to the
absence of `cc1`, this removes the `rust-cache` step. (Caching
would probably still confer some benefit, since it caches
dependencies. Even after running `cargo clean` between the builds,
most of these should be reacquired when building with `cc1`.
However, to make the whole thing easier to reason about, the step
is removed. It can be re-added if the job runs too slowly.)

This considers any `-sys` crate to be a dependency that needs to
build C. This is in principle not always the case, since they may
use existing shared library binaries, though there may still be
stub code that has to be built in C. The relevance of the new
steps varies depending on precisely how one conceptualizes "pure"
in `max-pure`, which is another reason for them to start out as
`continue-on-error`.

The `-sys` crates this finds in the `max-pure` dependency tree are:

- `libsqlite3-sys` via `rusqlite`. This was reported in GitoxideLabs#1681 and
  strongly seems to be unintended for `max-pure`.

- `linux-raw-sys` via `rustix` and `xattr`. It's less clear whether
  this should be considered impure, since its purpose is to
  interact with an operating system facility, and it may be
  comparable to the use of `libc` (on Unix-like systems). However,
  this does seem like it would not be able to build without a C
  compiler.

The dependency tree also has an occurrence of the `cc` crate
without a related `-sys` dependency, as required by `ring`. The
`ring` crate contains a C test program that is built when building
`ring`. That is currently the first build error shown in the output
of the "Do max-pure build without being able to compile C or C++"
step.
@Byron Byron added acknowledged an issue is accepted as shortcoming to be fixed blocked Issue can't progress due to external dependencies help wanted Extra attention is needed and removed blocked Issue can't progress due to external dependencies labels Nov 17, 2024
@Byron
Copy link
Member

Byron commented Nov 17, 2024

Thanks for bringing this to my attention.

It's true that max-pure is over-promising at the moment and what really is tested on CI is that a build is possible without additional tooling, like cmake. Here is the listing of prerequisites that are currently required to make any Rust build:

run: |
prerequisites=(
ca-certificates
curl
gcc # rustc calls gcc to invoke the linker.
libc-dev # rustc, in the toolchain we are using, dynamically links to the system libc.
)
apt-get update
apt-get install --no-install-recommends -y -- "${prerequisites[@]}"

Note that curl is used to install Rust via rustup.rs.

As of now, it's not possible to make any Rust build without a C compiler as it invokes the linker through it. Thanks to that, C-dependencies can always sneak in and build as long as they don't invoke more than make to compile.

Even though this particular issue can be solved by turning off gitoxide-core-tools-query for the max-pure build, it seems that what this issue really is about is to be able to build without a functional gcc.

Maybe the CI run could be tuned to replace gcc with a wrapper that only allows linking - then an attempt to build an actual C file would fail naturally, and one would maybe notice even more such invocations.
Maybe there is also a way to force Rust to use ld directly?

@NobodyXu
Copy link
Contributor

There's nightly flag for using rustup distributed lld, honestly I wish something like zig-cc is provided via rustup.

@Byron
Copy link
Member

Byron commented Nov 17, 2024

That should be possible to integrate on CI and would solve the problem. Thanks for suggesting!

@EliahKagan
Copy link
Member

EliahKagan commented Nov 17, 2024

I've opened #1682 to make the pure-rust-build CI job detect this issue, including but going beyond the dependency on libsqlite3-sys that was reported here (since there are some other dependencies that build C code as well).

The approach I've proposed there is intended as a way of getting immediate greater insight into how and why max-pure has dependencies that build C. I think it might also be useful alongside the approach proposed in #1681 (comment), or that it could potentially be replaced by that approach. Once this issue is fixed (or closer to being fixed), the benefit of allowing the build to succeed to observe all C compilation may no longer be needed.

@Byron Byron added the blocked Issue can't progress due to external dependencies label Nov 17, 2024
@Byron
Copy link
Member

Byron commented Nov 17, 2024

Thanks a lot for your help!

I have set this issue to blocked once again as ring currently builds some C code as part of its own compilation. It probably does so for very good reason, but while it's doing so a truly pure Rust build can't be accomplished unless one turns off HTTPS transport.

I could also imagine an intermediate step which involves setting up a pure target, one that tries to achieve a pure Rust build without having to provide max features.

@tanriol
Copy link
Author

tanriol commented Nov 17, 2024

Not that I personally needed it to be absolutely pure Rust - removing gitoxide-core-tools-query and gitoxide-core-tools-corpus from max-pure was enough for my case to work, everything else cross-compiled successfully - but that's what the feature naming implied.

Byron added a commit that referenced this issue Nov 17, 2024
That way, it won't require sqlite to be compiled, which was
part of the original idea.

It's worth noting that some C is still getting compiled right now
as part of `ring`.
@Byron Byron mentioned this issue Nov 17, 2024
2 tasks
@Byron
Copy link
Member

Byron commented Nov 17, 2024

Great to hear. Then I'd say I keep the pure and max-pure situation in mind and we call this issue resolved once max-pure doesn't pull in sqlite anymore.

Byron added a commit that referenced this issue Nov 17, 2024
That way, it won't require sqlite to be compiled, which was
part of the original idea.

It's worth noting that some C is still getting compiled right now
as part of `ring`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed blocked Issue can't progress due to external dependencies help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants