| Message ID | 20240906222116.270196-6-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
Calvin Wan <calvinwan@google.com> writes: > Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets > to their respective Makefiles so they can be built and tested without > having to run cargo build/test. I feel like clippy should be run as part of these somehow, but I'm not sure where. > +libgitrs-sys: > + $(QUIET)(\ > + cd contrib/libgit-rs/libgit-sys && \ > + cargo build \ > + ) > +.PHONY: libgitrs > +libgitrs: > + $(QUIET)(\ > + cd contrib/libgit-rs && \ > + cargo build \ > + ) We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to `-Wall` in the C world, no? These crates should build without warnings. Very excited to see this work; thank you for putting the time into it :-)
Sean Allred <allred.sean@gmail.com> writes: > Calvin Wan <calvinwan@google.com> writes: >> Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets >> to their respective Makefiles so they can be built and tested without >> having to run cargo build/test. > > I feel like clippy should be run as part of these somehow, but I'm not > sure where. > >> +libgitrs-sys: >> + $(QUIET)(\ >> + cd contrib/libgit-rs/libgit-sys && \ >> + cargo build \ >> + ) >> +.PHONY: libgitrs >> +libgitrs: >> + $(QUIET)(\ >> + cd contrib/libgit-rs && \ >> + cargo build \ >> + ) > > We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to > `-Wall` in the C world, no? These crates should build without warnings. > > Very excited to see this work; thank you for putting the time into it > :-) Thanks for helping.
On Sat, Sep 7, 2024 at 8:15 AM Sean Allred <allred.sean@gmail.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets > > to their respective Makefiles so they can be built and tested without > > having to run cargo build/test. > > I feel like clippy should be run as part of these somehow, but I'm not > sure where. > > > +libgitrs-sys: > > + $(QUIET)(\ > > + cd contrib/libgit-rs/libgit-sys && \ > > + cargo build \ > > + ) > > +.PHONY: libgitrs > > +libgitrs: > > + $(QUIET)(\ > > + cd contrib/libgit-rs && \ > > + cargo build \ > > + ) > > We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to > `-Wall` in the C world, no? These crates should build without warnings. > > Very excited to see this work; thank you for putting the time into it > :-) > > -- > Sean Allred Thanks for the suggestions! Will look into both for the next reroll.
On 2024-09-07 at 15:15:12, Sean Allred wrote: > Calvin Wan <calvinwan@google.com> writes: > > Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets > > to their respective Makefiles so they can be built and tested without > > having to run cargo build/test. > > I feel like clippy should be run as part of these somehow, but I'm not > sure where. Yes, that seems like a good idea in CI. > > +libgitrs-sys: > > + $(QUIET)(\ > > + cd contrib/libgit-rs/libgit-sys && \ > > + cargo build \ > > + ) > > +.PHONY: libgitrs > > +libgitrs: > > + $(QUIET)(\ > > + cd contrib/libgit-rs && \ > > + cargo build \ > > + ) > > We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to > `-Wall` in the C world, no? These crates should build without warnings. I believe -Dwarnings turns warnings into errors (at least it does in my tests), which is equivalent to -Werror. We don't want that because it breaks compiling older code with newer versions of the compiler, which makes it harder to bisect changes or compiler on the system compiler (or sometimes, other architectures or OSes). That would be fine for clippy, however, because that would only run in CI, where we _would_ want to catch newer changes, but we want to compile nonetheless.
On 2024.09.07 10:15, Sean Allred wrote: > Calvin Wan <calvinwan@google.com> writes: > > Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets > > to their respective Makefiles so they can be built and tested without > > having to run cargo build/test. > > I feel like clippy should be run as part of these somehow, but I'm not > sure where. > > > +libgitrs-sys: > > + $(QUIET)(\ > > + cd contrib/libgit-rs/libgit-sys && \ > > + cargo build \ > > + ) > > +.PHONY: libgitrs > > +libgitrs: > > + $(QUIET)(\ > > + cd contrib/libgit-rs && \ > > + cargo build \ > > + ) > > We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to > `-Wall` in the C world, no? These crates should build without warnings. > > Very excited to see this work; thank you for putting the time into it > :-) > > -- > Sean Allred Done in V4, thanks for the suggestion.
On 2024.09.13 19:01, brian m. carlson wrote: > On 2024-09-07 at 15:15:12, Sean Allred wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets > > > to their respective Makefiles so they can be built and tested without > > > having to run cargo build/test. > > > > I feel like clippy should be run as part of these somehow, but I'm not > > sure where. > > Yes, that seems like a good idea in CI. > > > > +libgitrs-sys: > > > + $(QUIET)(\ > > > + cd contrib/libgit-rs/libgit-sys && \ > > > + cargo build \ > > > + ) > > > +.PHONY: libgitrs > > > +libgitrs: > > > + $(QUIET)(\ > > > + cd contrib/libgit-rs && \ > > > + cargo build \ > > > + ) > > > > We should definitely be setting `RUSTFLAGS=-Dwarnings` as an analog to > > `-Wall` in the C world, no? These crates should build without warnings. > > I believe -Dwarnings turns warnings into errors (at least it does in my > tests), which is equivalent to -Werror. We don't want that because it > breaks compiling older code with newer versions of the compiler, which > makes it harder to bisect changes or compiler on the system compiler (or > sometimes, other architectures or OSes). > > That would be fine for clippy, however, because that would only run in > CI, where we _would_ want to catch newer changes, but we want to > compile nonetheless. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA Sorry, I missed your message before replying on this point last week. I'll remove this from our build.rs in V4.
diff --git a/Makefile b/Makefile index abeee01d9e..41ad458aef 100644 --- a/Makefile +++ b/Makefile @@ -3870,6 +3870,22 @@ build-unit-tests: $(UNIT_TEST_PROGS) unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X $(MAKE) -C t/ unit-tests +.PHONY: libgitrs-sys +libgitrs-sys: + $(QUIET)(\ + cd contrib/libgit-rs/libgit-sys && \ + cargo build \ + ) +.PHONY: libgitrs +libgitrs: + $(QUIET)(\ + cd contrib/libgit-rs && \ + cargo build \ + ) +ifdef INCLUDE_LIBGIT_RS +all:: libgitrs +endif + contrib/libgit-rs/libgit-sys/partial_symbol_export.o: contrib/libgit-rs/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a $(LD) -r $^ -o $@ diff --git a/t/Makefile b/t/Makefile index b2eb9f770b..066cb5c2b4 100644 --- a/t/Makefile +++ b/t/Makefile @@ -169,3 +169,19 @@ perf: .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \ check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS) + +.PHONY: libgitrs-sys-test +libgitrs-sys-test: + $(QUIET)(\ + cd ../contrib/libgit-rs/libgit-sys && \ + cargo test \ + ) +.PHONY: libgitrs-test +libgitrs-test: + $(QUIET)(\ + cd ../contrib/libgit-rs && \ + cargo test \ + ) +ifdef INCLUDE_LIBGIT_RS +all:: libgitrs-sys-test libgitrs-test +endif
Add libgitrs, libgitrs-sys, libgitrs-test, and libgitrs-sys-test targets to their respective Makefiles so they can be built and tested without having to run cargo build/test. Add environment variable, INCLUDE_LIBGIT_RS, that when set, automatically builds and tests libgit-rs and libgit-rs-sys when `make all` is ran. Signed-off-by: Calvin Wan <calvinwan@google.com> --- Makefile | 16 ++++++++++++++++ t/Makefile | 16 ++++++++++++++++ 2 files changed, 32 insertions(+)