| Message ID | cover.1723054623.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce cgit-rs, a Rust wrapper around libgit.a | expand |
On 2024-08-07 at 18:21:25, Josh Steadmon wrote: > We are putting error handling on hold for now since it is too complex > and we intend other CLIs to be our first customers, in which case > printing out errors is not the worst. While I think that's okay for now, that really needs to be one of the first priorities of things we fix. I have some ideas about an approach which we can take, which I'll send out in the next few days in a separate thread. > While the wrapper itself lives in contrib/, there are a couple of > patches that touch git.git code. These patches are necessary for the > wrapper, but not for git.git itself, which may seem unnecessary to > merge. However, I would argue that other languages (not just limited to > Rust) have issues calling functions that require a pointer to > non-generic objects and essentially require a redefinition in their own > language. I don't see a problem with this. It seems very self contained. > We're sending this series as RFC because there is remaining work > we'd like to do, but we'd like to get early feedback on this approach, > and particularly to ask for advice on a few topics: > > * alternative methods of exposing only a subset of symbols in our > library We should probably support symbol versioning in the libgit_ code on supported platforms if we're going to expose it. Otherwise, we're setting ourselves and distributors up for a world of pain when we change the ABI, such as when we add error handling. > * bikeshedding on the name (yes, really). There is an active, unrelated > CGit project [4] that we only recently became aware of. We originally > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > as "cgit" to distinguish it from jgit [5]. > > * gauging the level of interest in calling Git code from Rust I left some comments in the series. I think this is a nice first step as a proof of concept, and I'm very pleased to see it. If your goal is to simply expose C APIs, I suggest calling it something like cgit-sys, since the -sys suffix is customary for wrappers of C libraries. If your goal is to provide actual wrappers that people will use in real Rust code, then I think that we should _not_ expose the C API, since most Rust users will not want to use that.
On 2024.08.07 22:03, brian m. carlson wrote: > On 2024-08-07 at 18:21:25, Josh Steadmon wrote: > > We are putting error handling on hold for now since it is too complex > > and we intend other CLIs to be our first customers, in which case > > printing out errors is not the worst. > > While I think that's okay for now, that really needs to be one of the > first priorities of things we fix. I have some ideas about an approach > which we can take, which I'll send out in the next few days in a > separate thread. I'll be very interested in seeing this, please CC me. > > While the wrapper itself lives in contrib/, there are a couple of > > patches that touch git.git code. These patches are necessary for the > > wrapper, but not for git.git itself, which may seem unnecessary to > > merge. However, I would argue that other languages (not just limited to > > Rust) have issues calling functions that require a pointer to > > non-generic objects and essentially require a redefinition in their own > > language. > > I don't see a problem with this. It seems very self contained. > > > We're sending this series as RFC because there is remaining work > > we'd like to do, but we'd like to get early feedback on this approach, > > and particularly to ask for advice on a few topics: > > > > * alternative methods of exposing only a subset of symbols in our > > library > > We should probably support symbol versioning in the libgit_ code on > supported platforms if we're going to expose it. Otherwise, we're > setting ourselves and distributors up for a world of pain when we change > the ABI, such as when we add error handling. Symbol visibility and versioning are both areas I'm not very familiar with. I'll do some homework but might need additional help later on, so please keep an eye out for beginner mistakes I might make in later versions. > > * bikeshedding on the name (yes, really). There is an active, unrelated > > CGit project [4] that we only recently became aware of. We originally > > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > > as "cgit" to distinguish it from jgit [5]. > > > > * gauging the level of interest in calling Git code from Rust > > I left some comments in the series. I think this is a nice first step > as a proof of concept, and I'm very pleased to see it. > > If your goal is to simply expose C APIs, I suggest calling it something > like cgit-sys, since the -sys suffix is customary for wrappers of C > libraries. If your goal is to provide actual wrappers that people will > use in real Rust code, then I think that we should _not_ expose the C > API, since most Rust users will not want to use that. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
On 2024-08-07 at 22:03:53, brian m. carlson wrote: > I left some comments in the series. I think this is a nice first step > as a proof of concept, and I'm very pleased to see it. I noticed a couple of other things. First, the code has not been run through rustfmt. I think it would be helpful for us to do that since it makes it easier to not argue about style and it can be easily enforced in CI. It will also reduce diff noise, which I expect Junio will appreciate. Second, cargo clippy complains about some of the code. It's again helpful if we can fix those warnings or, if they're not appropriate, to disable them with an appropriate `allow` pragma. (In this case, I think they're both spot on, but I have seen some cases where I've disabled a warning.) This is something we may also want to test in CI in the future, and downstream users of our crate will appreciate not getting warnings when using clippy themselves, so we should be kind to them. I noticed these because my editor complains about the latter and I have now intuited enough of rustfmt's output that I can tell sometimes when things aren't formatted with it. For those members of the list who are less familiar with Rust, rustfmt is the standard code formatter (and formatting verifier) and clippy is a lint tool recommending best practices. Both are shipped with Rust and using both is customary for Rust projects.
Hi, Cgit maintainer here... On Wed, Aug 07, 2024 at 11:21:25AM -0700, Josh Steadmon wrote: > * bikeshedding on the name (yes, really). There is an active, unrelated > CGit project [4] that we only recently became aware of. We originally > took the name "cgit" because at $DAYJOB we sometimes refer to git.git > as "cgit" to distinguish it from jgit [5]. Indeed, please find something else. Cgit is a real project, used by many, such as git.kernel.org, and it'll turn into a real hassle for both of us if there's ambiguity and confusion. What about libgit-rs? Or even libgit3, where the rustness of it is simply an implementation detail, so the name won't feel dated 15 years from now when everything is written in Rust anyway and -rs is so 2020s? Jason
On 2024-08-08 13:51, Jason A. Donenfeld wrote: > Cgit maintainer here... > > On Wed, Aug 07, 2024 at 11:21:25AM -0700, Josh Steadmon wrote: >> * bikeshedding on the name (yes, really). There is an active, >> unrelated >> CGit project [4] that we only recently became aware of. We >> originally >> took the name "cgit" because at $DAYJOB we sometimes refer to >> git.git >> as "cgit" to distinguish it from jgit [5]. > > Indeed, please find something else. Cgit is a real project, used by > many, such as git.kernel.org, and it'll turn into a real hassle for > both > of us if there's ambiguity and confusion. Totally agreed, naming it cgit-rs makes pretty much no sense. > What about libgit-rs? Or even libgit3, where the rustness of it is > simply an implementation detail, so the name won't feel dated 15 years > from now when everything is written in Rust anyway and -rs is so 2020s? Well, there are still very active commercial projects written in COBOL or Clipper, for example, so I wouldn't go as far as _everything_ being written in Rust at some point.
On Thursday, August 8, 2024 9:59 AM, Dragan Simic wrote: >On 2024-08-08 13:51, Jason A. Donenfeld wrote: >> Cgit maintainer here... >> >> On Wed, Aug 07, 2024 at 11:21:25AM -0700, Josh Steadmon wrote: >>> * bikeshedding on the name (yes, really). There is an active, >>> unrelated >>> CGit project [4] that we only recently became aware of. We >>> originally >>> took the name "cgit" because at $DAYJOB we sometimes refer to >>> git.git >>> as "cgit" to distinguish it from jgit [5]. >> >> Indeed, please find something else. Cgit is a real project, used by >> many, such as git.kernel.org, and it'll turn into a real hassle for >> both of us if there's ambiguity and confusion. > >Totally agreed, naming it cgit-rs makes pretty much no sense. > >> What about libgit-rs? Or even libgit3, where the rustness of it is >> simply an implementation detail, so the name won't feel dated 15 years >> from now when everything is written in Rust anyway and -rs is so 2020s? > >Well, there are still very active commercial projects written in COBOL or Clipper, for >example, so I wouldn't go as far as _everything_ being written in Rust at some point. There are hundreds of millions of lines of code in the NonStop (TAL, COBOL, C, Java, in that order) community that is actively maintained to this day, with no Rust involvement (not for lack of trying to get Rust ported). Without these projects, most of your credit and debit cards would probably not work.
On 2024-08-08 17:38, rsbecker@nexbridge.com wrote: > On Thursday, August 8, 2024 9:59 AM, Dragan Simic wrote: >> On 2024-08-08 13:51, Jason A. Donenfeld wrote: >>> Cgit maintainer here... >>> >>> On Wed, Aug 07, 2024 at 11:21:25AM -0700, Josh Steadmon wrote: >>>> * bikeshedding on the name (yes, really). There is an active, >>>> unrelated >>>> CGit project [4] that we only recently became aware of. We >>>> originally >>>> took the name "cgit" because at $DAYJOB we sometimes refer to >>>> git.git >>>> as "cgit" to distinguish it from jgit [5]. >>> >>> Indeed, please find something else. Cgit is a real project, used by >>> many, such as git.kernel.org, and it'll turn into a real hassle for >>> both of us if there's ambiguity and confusion. >> >> Totally agreed, naming it cgit-rs makes pretty much no sense. >> >>> What about libgit-rs? Or even libgit3, where the rustness of it is >>> simply an implementation detail, so the name won't feel dated 15 >>> years >>> from now when everything is written in Rust anyway and -rs is so >>> 2020s? >> >> Well, there are still very active commercial projects written in COBOL >> or Clipper, for >> example, so I wouldn't go as far as _everything_ being written in Rust >> at some point. > > There are hundreds of millions of lines of code in the NonStop (TAL, > COBOL, C, Java, > in that order) community that is actively maintained to this day, with > no Rust > involvement (not for lack of trying to get Rust ported). Without these > projects, most > of your credit and debit cards would probably not work.
"Jason A. Donenfeld" <Jason@zx2c4.com> writes: > Cgit maintainer here... > > On Wed, Aug 07, 2024 at 11:21:25AM -0700, Josh Steadmon wrote: >> * bikeshedding on the name (yes, really). There is an active, unrelated >> CGit project [4] that we only recently became aware of. We originally >> took the name "cgit" because at $DAYJOB we sometimes refer to git.git >> as "cgit" to distinguish it from jgit [5]. > > Indeed, please find something else. Cgit is a real project, used by > many, such as git.kernel.org, and it'll turn into a real hassle for both > of us if there's ambiguity and confusion. > > What about libgit-rs? Or even libgit3, where the rustness of it is > simply an implementation detail, so the name won't feel dated 15 years > from now when everything is written in Rust anyway and -rs is so 2020s? libgit-rs sounds like a descriptive and good name. I am not sure about the wisdom of _not_ saying anything about Rust. In 2035 maybe nobody is writing in Rust but in not-yet-invented alternative that is even better.
Josh Steadmon <steadmon@google.com> writes: > We're sending this series as RFC because there is remaining work > we'd like to do, but we'd like to get early feedback on this approach, > and particularly to ask for advice on a few topics: I am not sure how much this is reusable, after seeing comments that "cgit-rs" may not be the best name for this thing and pathnames may have to change, but I needed the following merge-fix to get this into "seen" and have the result pass "make", due to interactions with the ps/config-wo-the-repository topic. contrib/cgit-rs/public_symbol_export.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/cgit-rs/public_symbol_export.c b/contrib/cgit-rs/public_symbol_export.c index 9641afca89..2732f5898e 100644 --- a/contrib/cgit-rs/public_symbol_export.c +++ b/contrib/cgit-rs/public_symbol_export.c @@ -9,6 +9,8 @@ #include "setup.h" #include "version.h" +extern struct repository *the_repository; + #pragma GCC visibility push(default) const char *libgit_setup_git_directory(void) @@ -18,7 +20,7 @@ const char *libgit_setup_git_directory(void) int libgit_config_get_int(const char *key, int *dest) { - return git_config_get_int(key, dest); + return repo_config_get_int(the_repository, key, dest); } void libgit_initialize_the_repository(void)
Junio C Hamano <gitster@pobox.com> writes: > Josh Steadmon <steadmon@google.com> writes: > >> We're sending this series as RFC because there is remaining work >> we'd like to do, but we'd like to get early feedback on this approach, >> and particularly to ask for advice on a few topics: > > I am not sure how much this is reusable, after seeing comments that > "cgit-rs" may not be the best name for this thing and pathnames may > have to change, but I needed the following merge-fix to get this > into "seen" and have the result pass "make", due to interactions > with the ps/config-wo-the-repository topic. In case you are wondering "so... is there anything actionable for *US*???", there isn't, exactly. But you'd need to holler if the "merge-fix" you saw in the message not correctly addressing the semantic clash between these two topics. Thanks. > > contrib/cgit-rs/public_symbol_export.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/contrib/cgit-rs/public_symbol_export.c b/contrib/cgit-rs/public_symbol_export.c > index 9641afca89..2732f5898e 100644 > --- a/contrib/cgit-rs/public_symbol_export.c > +++ b/contrib/cgit-rs/public_symbol_export.c > @@ -9,6 +9,8 @@ > #include "setup.h" > #include "version.h" > > +extern struct repository *the_repository; > + > #pragma GCC visibility push(default) > > const char *libgit_setup_git_directory(void) > @@ -18,7 +20,7 @@ const char *libgit_setup_git_directory(void) > > int libgit_config_get_int(const char *key, int *dest) > { > - return git_config_get_int(key, dest); > + return repo_config_get_int(the_repository, key, dest); > } > > void libgit_initialize_the_repository(void)
Junio C Hamano <gitster@pobox.com> writes: > Josh Steadmon <steadmon@google.com> writes: > >> We're sending this series as RFC because there is remaining work >> we'd like to do, but we'd like to get early feedback on this approach, >> and particularly to ask for advice on a few topics: > > I am not sure how much this is reusable, after seeing comments that > "cgit-rs" may not be the best name for this thing and pathnames may > have to change, but I needed the following merge-fix to get this > into "seen" and have the result pass "make", due to interactions > with the ps/config-wo-the-repository topic. > > contrib/cgit-rs/public_symbol_export.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) There is another thing. Listing this file in $(OBJECTS) means that you should be able to pass "make sparse" to build contrib/cgit-rs/public_symbol_export.sp but it seems to fail rather miserably. I am tempted to suggest in the meantime to futz with $(SP_OBJ) to filter it out in the top level Makefile.
On 2024.08.08 01:33, brian m. carlson wrote: > On 2024-08-07 at 22:03:53, brian m. carlson wrote: > > I left some comments in the series. I think this is a nice first step > > as a proof of concept, and I'm very pleased to see it. > > I noticed a couple of other things. First, the code has not been run > through rustfmt. I think it would be helpful for us to do that since it > makes it easier to not argue about style and it can be easily enforced > in CI. It will also reduce diff noise, which I expect Junio will > appreciate. > > Second, cargo clippy complains about some of the code. It's again > helpful if we can fix those warnings or, if they're not appropriate, to > disable them with an appropriate `allow` pragma. (In this case, I think > they're both spot on, but I have seen some cases where I've disabled a > warning.) This is something we may also want to test in CI in the > future, and downstream users of our crate will appreciate not getting > warnings when using clippy themselves, so we should be kind to them. > > I noticed these because my editor complains about the latter and I have > now intuited enough of rustfmt's output that I can tell sometimes when > things aren't formatted with it. > > For those members of the list who are less familiar with Rust, rustfmt > is the standard code formatter (and formatting verifier) and clippy is a > lint tool recommending best practices. Both are shipped with Rust and > using both is customary for Rust projects. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA Applied Clippy and rustfmt fixes throughout V2, thanks for the catch.
Junio C Hamano <gitster@pobox.com> writes: >> contrib/cgit-rs/public_symbol_export.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) > > There is another thing. > > Listing this file in $(OBJECTS) means that you should be able to > pass "make sparse" to build contrib/cgit-rs/public_symbol_export.sp > but it seems to fail rather miserably. I am tempted to suggest in > the meantime to futz with $(SP_OBJ) to filter it out in the top > level Makefile. FWIW, this one _is_ actionable on your end ;-) Make sure "make sparse" does not barf due to the addition of libgit-rust.
On 2024.08.09 12:29, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Josh Steadmon <steadmon@google.com> writes: > > > >> We're sending this series as RFC because there is remaining work > >> we'd like to do, but we'd like to get early feedback on this approach, > >> and particularly to ask for advice on a few topics: > > > > I am not sure how much this is reusable, after seeing comments that > > "cgit-rs" may not be the best name for this thing and pathnames may > > have to change, but I needed the following merge-fix to get this > > into "seen" and have the result pass "make", due to interactions > > with the ps/config-wo-the-repository topic. > > In case you are wondering "so... is there anything actionable for > *US*???", there isn't, exactly. But you'd need to holler if the > "merge-fix" you saw in the message not correctly addressing the > semantic clash between these two topics. > > Thanks. Looks fine to me, applied in V2.
On 2024.08.09 13:54, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Josh Steadmon <steadmon@google.com> writes: > > > >> We're sending this series as RFC because there is remaining work > >> we'd like to do, but we'd like to get early feedback on this approach, > >> and particularly to ask for advice on a few topics: > > > > I am not sure how much this is reusable, after seeing comments that > > "cgit-rs" may not be the best name for this thing and pathnames may > > have to change, but I needed the following merge-fix to get this > > into "seen" and have the result pass "make", due to interactions > > with the ps/config-wo-the-repository topic. > > > > contrib/cgit-rs/public_symbol_export.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > There is another thing. > > Listing this file in $(OBJECTS) means that you should be able to > pass "make sparse" to build contrib/cgit-rs/public_symbol_export.sp > but it seems to fail rather miserably. I am tempted to suggest in > the meantime to futz with $(SP_OBJ) to filter it out in the top > level Makefile. I believe that I fixed `make sparse` (at least in GitHub CI, it fails for seemingly unrelated reasons on my desktop) by removing some unnecessarily exposed symbols in public_symbol_export.c. If it still fails for you in V2, please let me know.
Josh Steadmon <steadmon@google.com> writes: > On 2024.08.09 13:54, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >> > Josh Steadmon <steadmon@google.com> writes: >> > >> >> We're sending this series as RFC because there is remaining work >> >> we'd like to do, but we'd like to get early feedback on this approach, >> >> and particularly to ask for advice on a few topics: >> > >> > I am not sure how much this is reusable, after seeing comments that >> > "cgit-rs" may not be the best name for this thing and pathnames may >> > have to change, but I needed the following merge-fix to get this >> > into "seen" and have the result pass "make", due to interactions >> > with the ps/config-wo-the-repository topic. >> > >> > contrib/cgit-rs/public_symbol_export.c | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> >> There is another thing. >> >> Listing this file in $(OBJECTS) means that you should be able to >> pass "make sparse" to build contrib/cgit-rs/public_symbol_export.sp >> but it seems to fail rather miserably. I am tempted to suggest in >> the meantime to futz with $(SP_OBJ) to filter it out in the top >> level Makefile. > > I believe that I fixed `make sparse` (at least in GitHub CI, it fails > for seemingly unrelated reasons on my desktop) by removing some > unnecessarily exposed symbols in public_symbol_export.c. If it still > fails for you in V2, please let me know. Thanks. Please let me know when you send v2 out ;-)