#50633 Add cargo vendor support for offline builds
Closed 3 years ago by spichugi. Opened 4 years ago by firstyear.
firstyear/389-ds-base 20191003-rust-vendor  into  master

file added
+8
@@ -0,0 +1,8 @@ 

+ 

+ 

+ [source.crates-io]

+ replace-with = "vendored-sources"

+ 

+ [source.vendored-sources]

+ directory = "./vendor"

+ 

file modified
+2
@@ -225,8 +225,10 @@ 

  src/lib389/dist/

  src/lib389/man/

  src/libsds/target/

+ src/librslapd/target/

  dist

  venv

  .idea

  src/cockpit/389-console/cockpit_dist/

  src/cockpit/389-console/node_modules/

+ ldap/servers/slapd/rust-slapi-private.h

file modified
+60 -6
@@ -33,12 +33,19 @@ 

  

  # Rust inclusions.

  if RUST_ENABLE

+ # Rust enabled

  RUST_ON = 1

  CARGO_FLAGS = @cargo_defs@

  RUSTC_FLAGS = @asan_rust_defs@ @msan_rust_defs@ @tsan_rust_defs@ @debug_rust_defs@

  RUST_LDFLAGS = -ldl -lpthread -lgcc_s -lc -lm -lrt -lutil

  RUST_DEFINES = -DRUST_ENABLE

+ if RUST_ENABLE_OFFLINE

+ RUST_OFFLINE = --locked --offline

  else

+ RUST_OFFLINE =

+ endif

+ else

+ # Rust disabled

  RUST_ON = 0

  CARGO_FLAGS =

  RUSTC_FLAGS =
@@ -211,6 +218,10 @@ 

  BUILT_SOURCES = dberrstrs.h \

  	$(POLICY_FC)

  

+ if RUST_ENABLE

+ BUILT_SOURCES += rust-slapi-private.h

+ endif

+ 

  if enable_posix_winsync

  LIBPOSIX_WINSYNC_PLUGIN = libposix-winsync-plugin.la

  endif
@@ -269,6 +280,10 @@ 

  	doxyfile.stamp ldap/admin/src/scripts/dbmon.sh \

  	$(NULL)

  

+ if RUST_ENABLE

+ CLEANFILES += rust-slapi-private.h

+ endif

+ 

  clean-local:

  	-rm -rf dist

  	-rm -rf $(abs_top_builddir)/html
@@ -1172,7 +1187,7 @@ 

  

  if RUST_ENABLE

  

- noinst_LTLIBRARIES = librsds.la

+ noinst_LTLIBRARIES = librsds.la librslapd.la

  

  ### Why does this exist?

  #
@@ -1181,6 +1196,8 @@ 

  # https://people.gnome.org/~federico/blog/librsvg-build-infrastructure.html

  # https://gitlab.gnome.org/GNOME/librsvg/blob/master/Makefile.am

  

+ ### Rust datastructures

+ 

  RSDS_LIB = @abs_top_builddir@/rs/@rust_target_dir@/librsds.a

  

  libsds_la_LIBADD = $(RSDS_LIB)
@@ -1193,15 +1210,47 @@ 

  librsds_la_EXTRA = src/libsds/Cargo.lock

  

  @abs_top_builddir@/rs/@rust_target_dir@/librsds.a: $(librsds_la_SOURCES)

- 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs RUSTC_BOOTSTRAP=1 \

- 		cargo rustc --manifest-path=$(srcdir)/src/libsds/Cargo.toml \

+ 	RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \

+ 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs \

+ 		cargo rustc $(RUST_OFFLINE) --manifest-path=$(srcdir)/src/libsds/Cargo.toml \

  		$(CARGO_FLAGS) --verbose -- $(RUSTC_FLAGS)

  

- EXTRA_DIST = $(librsds_la_SOURCES) $(librsds_la_EXTRA)

+ ### Rust lib slapd components

+ RSLAPD_LIB = @abs_top_builddir@/rs/@rust_target_dir@/librslapd.a

+ 

+ librslapd_la_SOURCES = \

+ 	src/librslapd/Cargo.toml \

+ 	src/librslapd/build.rs \

+ 	src/librslapd/src/lib.rs

+ 

+ librslapd_la_EXTRA = src/librslapd/Cargo.lock

  

+ @abs_top_builddir@/rs/@rust_target_dir@/librslapd.a: $(librslapd_la_SOURCES)

+ 	RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \

+ 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs \

+ 	SLAPD_HEADER_DIR=$(abs_top_builddir)/ \

+ 		cargo rustc $(RUST_OFFLINE) --manifest-path=$(srcdir)/src/librslapd/Cargo.toml \

+ 		$(CARGO_FLAGS) --verbose -- $(RUSTC_FLAGS)

+ 

+ # The header needs the lib build first.

+ rust-slapi-private.h: @abs_top_builddir@/rs/@rust_target_dir@/librslapd.a

+ 

+ EXTRA_DIST = $(librsds_la_SOURCES) $(librsds_la_EXTRA) \

+ 			$(librslapd_la_SOURCES) $(librslapd_la_EXTRA)

+ 

+ ## Run rust tests

+ # cargo does not support offline tests :(

+ if RUST_ENABLE_OFFLINE

+ else

  check-local:

- 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs RUSTC_BOOTSTRAP=1 \

- 		cargo test --manifest-path=$(srcdir)/src/libsds/Cargo.toml

+ 	RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \

+ 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs \

+ 		cargo test $(RUST_OFFLINE) --manifest-path=$(srcdir)/src/libsds/Cargo.toml

+ 	RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \

+ 	CARGO_TARGET_DIR=$(abs_top_builddir)/rs \

+ 	SLAPD_HEADER_DIR=$(abs_top_builddir)/ \

+ 		cargo test $(RUST_OFFLINE) --manifest-path=$(srcdir)/src/librslapd/Cargo.toml

+ endif

  

  else

  # Just build the tqueue in C.
@@ -1363,6 +1412,11 @@ 

  

  libslapd_la_CPPFLAGS = $(AM_CPPFLAGS) $(DSPLUGIN_CPPFLAGS) $(SASL_CFLAGS) @db_inc@ $(KERBEROS_CFLAGS) $(PCRE_CFLAGS) $(SDS_CPPFLAGS) $(SVRCORE_INCLUDES)

  libslapd_la_LIBADD = $(LDAPSDK_LINK) $(SASL_LINK) $(NSS_LINK) $(NSPR_LINK) $(KERBEROS_LIBS) $(PCRE_LIBS) $(THREADLIB) $(SYSTEMD_LIBS) libsds.la libsvrcore.la

+ 

+ if RUST_ENABLE

+ libslapd_la_LIBADD += $(RSLAPD_LIB)

+ endif

+ 

  libslapd_la_LDFLAGS = $(AM_LDFLAGS) $(SLAPD_LDFLAGS)

  

  

file modified
+10 -2
@@ -85,19 +85,27 @@ 

  LT_LIB_DLLOAD

  

  # Optional rust component support.

+ AC_MSG_CHECKING(for --enable-rust-offline)

+ AC_ARG_ENABLE(rust_offline, AS_HELP_STRING([--enable-rust-offline], [Enable rust building offline. you MUST have run vendor! (default: no)]),

+               [], [ enable_rust_offline=no ])

+ AC_MSG_RESULT($enable_rust_offline)

+ AM_CONDITIONAL([RUST_ENABLE_OFFLINE],[test "$enable_rust_offline" = yes])

+ 

  AC_MSG_CHECKING(for --enable-rust)

  AC_ARG_ENABLE(rust, AS_HELP_STRING([--enable-rust], [Enable rust language features (default: no)]),

                [], [ enable_rust=no ])

  AC_MSG_RESULT($enable_rust)

- if test "$enable_rust" = yes ; then

+ if test "$enable_rust" = yes -o "$enable_rust_offline" = yes; then

      AC_CHECK_PROG(CARGO, [cargo], [yes], [no])

      AC_CHECK_PROG(RUSTC, [rustc], [yes], [no])

  

      AS_IF([test "$CARGO" != "yes" -o "$RUSTC" != "yes"], [

        AC_MSG_FAILURE("Rust based plugins cannot be built cargo=$CARGO rustc=$RUSTC")

      ])

+ 

+ 

  fi

- AM_CONDITIONAL([RUST_ENABLE],[test "$enable_rust" = yes])

+ AM_CONDITIONAL([RUST_ENABLE],[test "$enable_rust" = yes -o "$enable_rust_offline" = yes])

  

  AC_MSG_CHECKING(for --enable-debug)

  AC_ARG_ENABLE(debug, AS_HELP_STRING([--enable-debug], [Enable debug features (default: no)]),

@@ -133,6 +133,11 @@ 

  #endif

  #include <sys/resource.h>

  

+ #ifdef RUST_ENABLE

+ #include <rust-slapi-private.h>

+ #endif

+ 

+ 

  #define REMOVE_CHANGELOG_CMD "remove"

  

  int slapd_ldap_debug = SLAPD_DEFAULT_ERRORLOG_LEVEL;
@@ -1533,6 +1538,11 @@ 

      struct rlimit rlp;

      int64_t maxdescriptors = SLAPD_DEFAULT_MAXDESCRIPTORS;

  

+ #ifdef RUST_ENABLE

+     /* prove rust is working */

+     PR_ASSERT(do_nothing_rust() == 0);

+ #endif

+ 

  #if SLAPI_CFG_USE_RWLOCK == 1

      /* initialize the read/write configuration lock */

      if ((cfg->cfg_rwlock = slapi_new_rwlock()) == NULL) {

file modified
+11 -1
@@ -32,13 +32,23 @@ 

  	rm -rf dist

  	rm -rf rpmbuild

  

+ update-cargo-dependencies:

+ 	cargo update --manifest-path=./src/libsds/Cargo.toml

+ 	cargo update --manifest-path=./src/librslapd/Cargo.toml

+ 

+ download-cargo-dependencies:

+ 	cargo vendor --manifest-path=./src/libsds/Cargo.toml

+ 	cargo fetch --manifest-path=./src/libsds/Cargo.toml

+ 	cargo vendor --manifest-path=./src/librslapd/Cargo.toml

+ 	cargo fetch --manifest-path=./src/librslapd/Cargo.toml

+ 

  install-node-modules:

  	cd src/cockpit/389-console; make -f node_modules.mk install

  

  build-cockpit: install-node-modules

  	cd src/cockpit/389-console; make -f node_modules.mk build-cockpit-plugin

  

- dist-bz2: install-node-modules

+ dist-bz2: install-node-modules download-cargo-dependencies

  	cd src/cockpit/389-console; \

  	rm -rf cockpit_dist; \

  	make -f node_modules.mk build-cockpit-plugin; \

@@ -0,0 +1,24 @@ 

+ [package]

+ name = "librslapd"

+ version = "0.1.0"

+ authors = ["William Brown <william@blackhats.net.au>"]

+ edition = "2018"

+ build = "build.rs"

+ 

+ # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

+ 

+ [lib]

+ path = "src/lib.rs"

+ name = "rslapd"

+ crate-type = ["staticlib", "lib"]

+ 

+ [profile.release]

+ panic = "abort"

+ lto = true

+ 

+ 

+ [dependencies]

+ 

+ [build-dependencies]

+ cbindgen = "0.9"

+ 

@@ -0,0 +1,15 @@ 

+ extern crate cbindgen;

+ 

+ use std::env;

+ 

+ fn main() {

+     let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();

+     let out_dir = env::var("SLAPD_HEADER_DIR").unwrap();

+ 

+     cbindgen::Builder::new()

+         .with_language(cbindgen::Language::C)

+         .with_crate(crate_dir)

+         .generate()

+         .expect("Unable to generate bindings")

+         .write_to_file(format!("{}/rust-slapi-private.h", out_dir));

+ }

@@ -0,0 +1,12 @@ 

+ #[no_mangle]

+ pub extern "C" fn do_nothing_rust() -> usize {

+     0

+ }

+ 

+ #[cfg(test)]

+ mod tests {

+     #[test]

+     fn it_works() {

+         assert_eq!(2 + 2, 4);

+     }

+ }

file modified
+3 -1
@@ -1,7 +1,8 @@ 

  [package]

  name = "rsds"

  version = "0.1.0"

- authors = ["William Brown <firstyear@redhat.com>"]

+ authors = ["William Brown <william@blackhats.net.au>"]

+ edition = "2018"

  

  [dependencies]

  
@@ -14,3 +15,4 @@ 

  panic = "abort"

  lto = true

  

+ 

Unnecessary spacing.

Bug Description: At suse/rh we need to be able to build offline. To
achieve this we need offline builds. This adds support for these in
389-ds with cargo and rust.

Fix Description:
This adds cargo vendor support for offline builds,
and shows that they work. We add a stub library for librslapd/libslapd
so that we can begin to develop features in rust.

To build normally: work as usual.

To build offline: make -f rpm.mk download-cargo-dependencies
./configure --enable-rust --enable-rust-offline

Continue to build as usual.

A note to keep in mind is cargo test does not work offline as
dev-dependencies are not vendored.

The download-cargo-dependencies has been added to dist-bz2 for
distributions.

rebased onto ee63b8a130b295ff0c8ac23eff4e4f7fcdd10dd4

4 years ago

For npm packages we run npm audit-ci during tarball generation to prevent including packages with known vulnerabilities. It would be nice to do the same for cargo packages using cargo audit.

I have no experience with cargo audit at the moment - one step at a time please!

I still have to fix an issue where symbols from the rust libs can't be used in ns-slapd (only inside of libslapd right now), so there is to be some more churn here I expect.

Did you want this to be a whole commit of everything or did we want to do this in steps such as this with vendor, another step with audit, and finally a step that fixes the ns-slapd linking? I'm okay either way.

1 new commit added

  • Update to support testing with --offline --locked
4 years ago

I have no experience with cargo audit at the moment - one step at a time please!

That's why I said it'd be nice to have, not must to have. It would be must have once we start building with rust by default in downstream, as we want to protect from supply chain attacks.

Did you want this to be a whole commit of everything or did we want to do this in steps such as this with vendor, another step with audit, and finally a step that fixes the ns-slapd linking? I'm okay either way.

I'm fine having it separately as long as it's not breaking builds with current defaults (i.e. without rust).

Testing with HEAD at 1392b272.

First, mere autoreconf -fiv && ./configure --enable-rust && make gives me this (hint: it does no Rust compilation beforehand):

ldap/servers/slapd/libglobs.c:137:10: fatal error: rust-slapi-private.h: No such file or directory
  137 | #include "rust-slapi-private.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~

Second, I would fix this, otherwise it says it does not know the option:

diff --git a/configure.ac b/configure.ac
index b6eae5b4b..4e13b776f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -102,7 +102,7 @@ fi
 AM_CONDITIONAL([RUST_ENABLE],[test "$enable_rust" = yes])

 AC_MSG_CHECKING(for --enable-rust-offline)
-AC_ARG_ENABLE(rust, AS_HELP_STRING([--enable-rust-offline], [Enable rust building offline. you MUST have run vendor! (default: no)]),
+AC_ARG_ENABLE(rust_offline, AS_HELP_STRING([--enable-rust-offline], [Enable rust building offline. you MUST have run vendor! (default: no)]),
               [], [ enable_rust_offline=no ])
 AC_MSG_RESULT($enable_rust_offline)
 AM_CONDITIONAL([RUST_ENABLE_OFFLINE],[test "$enable_rust_offline" = yes])

but even then, the autoreconf -fiv && make -f rpm.mk download-cargo-dependencies && ./configure --enable-rust --enable-rust-offline && make does not work failing as in the First. Am I doing something wrong?

I wonder if I forgot an ifdef around that header maybe?

So you suggest moving the --enable-rust-offline to outsideo of the enable-rust ac_arg?

Ahhhh the header is generated from build.rs, that's why it's not working. Okay need to think about how to approach this.

1 new commit added

  • Fix up issues that Matus found in build with clean env
4 years ago

This commit should fix your issue :)

Unnecessary spacing.

Sorry for the delay. Here's couple of comments.

--enable-rust fails

I think it should not rely on offline vendor present:

perl ./ldap/servers/slapd/mkDBErrStrs.pl -i /usr/include/libdb -o .
RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \
CARGO_TARGET_DIR=/home/vagrant/ds/rs \
SLAPD_HEADER_DIR=/home/vagrant/ds/ \
    cargo rustc  --manifest-path=./src/librslapd/Cargo.toml \
    --release --verbose -- -C debuginfo=2
error: failed to load source for a dependency on `cbindgen`

Caused by:
  Unable to update registry `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to update replaced source registry `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to read root of directory source: /home/vagrant/ds/vendor

Caused by:
  No such file or directory (os error 2)
make: *** [Makefile:14571: /home/vagrant/ds/rs/release/librslapd.a] Error 101

./configure --enable-rust-offline

Does not seem it actually builds the Rust stuff at all. This is unexpected. AFAIU this is a toggle for when already building with Rust -- in this case I would guard (i.e. fail if incorrect) the usage of this flag by a requirement to have the --enable-rust flag enabled as well.

./configure --enable-rust --enable-rust-offline

Without pre-downloading using the make -f rpm.mk download-cargo-dependencies this fails with:

CARGO_TARGET_DIR=/home/vagrant/ds/rs \
SLAPD_HEADER_DIR=/home/vagrant/ds/ \
    cargo rustc --locked --offline --manifest-path=./src/librslapd/Cargo.toml \
    --release --verbose -- -C debuginfo=2
error: can't update in the offline mode
make: *** [Makefile:14571: /home/vagrant/ds/rs/release/librslapd.a] Error 101

I think we should fail at ./configure time when building with --enable-rust-offline and do not have /vendor prerequisites already downloaded, saying "You should download the prerequisites using make -f rpm.mk download-cargo-dependencies, first."

Finally, given the prerequisites have been pre-downloaded, the things seem to build just OK. :)

Couple of other issues

.gitignore -- missing vendor folder

Please add /vendor in there.

SPEC file not updated

We should probably address this in a separate ticket. William, could you please open a simple ticket for addressing these Rust changes in the SPEC file? Also, it would be great if you could link to e.g. the SUSE's SPEC file on how to actually ship this (even though it's still just a PoC).

Sorry for the delay. Here's couple of comments.
--enable-rust fails
I think it should not rely on offline vendor present:
perl ./ldap/servers/slapd/mkDBErrStrs.pl -i /usr/include/libdb -o .
RUST_BACKTRACE=1 RUSTC_BOOTSTRAP=1 \
CARGO_TARGET_DIR=/home/vagrant/ds/rs \
SLAPD_HEADER_DIR=/home/vagrant/ds/ \
cargo rustc --manifest-path=./src/librslapd/Cargo.toml \
--release --verbose -- -C debuginfo=2
error: failed to load source for a dependency on cbindgen

Caused by:
Unable to update registry https://github.com/rust-lang/crates.io-index

Caused by:
failed to update replaced source registry https://github.com/rust-lang/crates.io-index

Caused by:
failed to read root of directory source: /home/vagrant/ds/vendor

Ahhh because the .cargo/config is there so it thinks it needs vendoring always, not just when you use --offline. Errghh. Sounds like yet another issue for me to raise on cargo.

So far I'm finding this whole vendoring process really incomplete.

Caused by:
No such file or directory (os error 2)
make: *** [Makefile:14571: /home/vagrant/ds/rs/release/librslapd.a] Error 101

./configure --enable-rust-offline
Does not seem it actually builds the Rust stuff at all. This is unexpected. AFAIU this is a toggle for when already building with Rust -- in this case I would guard (i.e. fail if incorrect) the usage of this flag by a requirement to have the --enable-rust flag enabled as well.

Yep, that's a good comment, I can fix this easily. .

./configure --enable-rust --enable-rust-offline
Without pre-downloading using the make -f rpm.mk download-cargo-dependencies this fails with:
CARGO_TARGET_DIR=/home/vagrant/ds/rs \
SLAPD_HEADER_DIR=/home/vagrant/ds/ \
cargo rustc --locked --offline --manifest-path=./src/librslapd/Cargo.toml \
--release --verbose -- -C debuginfo=2
error: can't update in the offline mode
make: *** [Makefile:14571: /home/vagrant/ds/rs/release/librslapd.a] Error 101

I think we should fail at ./configure time when building with --enable-rust-offline and do not have /vendor prerequisites already downloaded, saying "You should download the prerequisites using make -f rpm.mk download-cargo-dependencies, first."
Finally, given the prerequisites have been pre-downloaded, the things seem to build just OK. :)

Yep, we could do a test for the folder,

Couple of other issues
.gitignore -- missing vendor folder
Please add /vendor in there.
SPEC file not updated

I didn't want to update the spec yet because I didn't know how we wanted to tackle making rust a requirement for RHEL/SUSE/Others yet.

We should probably address this in a separate ticket. William, could you please open a simple ticket for addressing these Rust changes in the SPEC file? Also, it would be great if you could link to e.g. the SUSE's SPEC file on how to actually ship this (even though it's still just a PoC).

Sure. I think that ticket would be "include rust by default" but good to have a ticket + spec changes theres.

So it looks like there is no variables in configure.ac for the source directory, which means it's not possible to check if vendor directory exists until we get to make, and we already error in various ways there. It's not perfect, but I think I can't solve that.

I've updated so that --enable-rust-offline will also set RUST_ENABLE to true.

Finally, see https://pagure.io/389-ds-base/issue/50662

rebased onto 77219e8c76122f7ace3714f869c52b10aa624bb5

4 years ago

So offline works, but as your note from 7520 there is issues with online build with the .cargo/config present. For now we just have to advise that people run: cd 389-ds; make -f rpm.mk download-cargo-dependencies first

@mhonek any further comments here? I think we should merge what we have because we're waiting on upstream to improve cargo at this point.

Mate, I have not elaborated enough to prove, yet, however I think it would be posible to actually create the .cargo/config as an artifact when building with --enable-rust-offline thus not having it as an obstacle when building online. Also, maybe also moving the two targets from rpm.mk to Makefile.am to be run as dependencies when --enable-rust-offline is used, since not running them in this case does not make sense.

However, if you're blocked by this (sorry :/), then go ahead with merging. We can do adjustments later.

Well, if we put vendoring into --enable-rust-offline that would kind of create a chicken and egg problem right?

So having vendoring external to the build makes sense. If anything the .cargo/config should be generated by the rpm.mk in that case (and possibly removed by makefile if online)?.

Anyway, I'll rebase and merge this for now.

rebased onto cc8bfec

4 years ago

Pull-Request has been merged by firstyear

4 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3688

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago