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

libgit.el: Be able to load libegit2 from load-path if found #126

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

Thaodan
Copy link
Contributor

@Thaodan Thaodan commented Oct 19, 2023

Prefer previously installed libegit2 but check first for libegit2 from
load-path if it wasn't build already.

I modeled this logic similarly as in Jinx or ZMQ.el

I'm going to package libgit, loading it from load-parh allows to prebuild libegit.so without patching the package.

Prefer previously installed libegit2 but check first for libegit2 from
load-path if it wasn't build already.

I modeled this logic similarly as in Jinx or ZMQ.el

Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
@Thaodan
Copy link
Contributor Author

Thaodan commented Oct 19, 2023

I just noticed that earlier that we build so because
before Emacs 28 module-suffix was so on MacOS.

Would it make sense to override the suffix for Emacs <28
on MacOS?

There is no standard directory for dynamic modules to be installed in.
If the target system uses a different path DYNMODDRIR has to be reset
during build.

Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
If the dependency for libegit2 is phony libegit2 is rebuild each time
make is ran despite it not having changed.

Signed-off-by: Björn Bidar <bjorn.bidar@thaodan.de>
@tarsius
Copy link
Member

tarsius commented Oct 22, 2023

Thanks, that sounds useful. I am still quite busy, so it will probably be a week until I get around to reviewing this.

@Thaodan
Copy link
Contributor Author

Thaodan commented Oct 23, 2023

Thanks, that sounds useful. I am still quite busy, so it will probably be a week until I get around to reviewing this.

No problem. If you can meanwhile give a comment on my comment above that would be great.

@tarsius
Copy link
Member

tarsius commented Oct 24, 2023

If you can meanwhile give a comment on my comment above that would be great.

Sure, but I am afraid the answer is "I have no idea".

@tarsius
Copy link
Member

tarsius commented Oct 24, 2023

As you (but not others) know, there is/was some related talk in 77bd28a.

@Thaodan
Copy link
Contributor Author

Thaodan commented Oct 30, 2023

I submitted libgit using these patches to openSUSE:
https://build.opensuse.org/package/show/editors/emacs-libgit2

@tarsius
Copy link
Member

tarsius commented Oct 30, 2023

IMO that was premature. You only opened this pull-request two weeks ago, and I have not rejected it. Sure, in an ideal world I would have reviewed it by now, but alas I was too busy for that. It seems to me, that by suggesting to the opensuse maintainers to apply these patches, you are just causing them more work. If they prioritize working on your proposal, then other things will get delayed. I.e., you are making the world a little less ideal, by generating work, which could likely be avoided altogether, just by being a bit more patient.

@Thaodan
Copy link
Contributor Author

Thaodan commented Oct 30, 2023 via email

@tarsius
Copy link
Member

tarsius commented Oct 30, 2023

I submitted the package to SUSE, it did not exist before and will maintain the package.

Ah, sorry I didn't realize that.

I did patch the package earlier to load the native module correctly, as policy patches should be if possible submitted to upstream.

So you did everything right. Maybe a little too much in a hurry, but that's okay.

Again sorry for the friction created.

It's all good. And sorry for overlooking some important detail. ;P

@tarsius tarsius merged commit 373defa into emacsorphanage:main Dec 4, 2023
@tarsius
Copy link
Member

tarsius commented Dec 4, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants