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

elisp-common.eclass: add support for nativecomp #16962

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgbugs
Copy link
Contributor

@tgbugs tgbugs commented Aug 2, 2020

This PR is not ready yet, but is here to start discussion.

The basic logic seems to be working at this point but the actual
compiling seems to encounter issues and does not fail correctly.
I also suspect that elisp-install may need to be made aware of the
fact that the eln folder may or may not be present and that it should
be installed in its entirety.

  • Emacs version: 28.0.50
  • Compiling GNU Emacs Elisp files ... [ ok ]
  • Native compiling GNU Emacs Elisp files ...
    ld: cannot find crtbeginS.o: No such file or directory
    ld: cannot find -lgcc
    ld: cannot find -lgcc_s
    libgccjit.so: error: error invoking gcc driver [ ok ]

Closes: https://bugs.gentoo.org/735148

Signed-off-by: Tom Gillespie tgbugs@gmail.org

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @tgbugs
Areas affected: eclasses
Packages affected: (none)

@gentoo/github

Linked bugs

Bugs linked: 735148

New packages

This Pull Request appears to be introducing new packages only. Due to limited manpower, adding new packages is considered low priority. This does not mean that your Pull Request will not receive any attention, however, it might take quite some time for it to be reviewed. In the meantime, your new ebuild might find a home in the GURU project repository: the ebuild repository maintained collaboratively by Gentoo users. GURU offers your ebuild a place to be reviewed and improved by other Gentoo users, while making it easy for Gentoo users to install it and enjoy the software it adds.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added need assignment It was impossible to assign the PR correctly. Please assign it manually. bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Aug 2, 2020
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-02 21:39 UTC
Newest commit scanned: dd87e95
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/26597cef18/output.html

@tgbugs
Copy link
Contributor Author

tgbugs commented Aug 2, 2020

@ulm I think you are the right person to ping about this.

Copy link
Member

@ulm ulm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS nativecomp hasn't been merged to the upstream master branch yet?

So this is certainly not ready for inclusion in the gentoo repo. However, we could think about testing it in the emacs overlay. https://gitweb.gentoo.org/repo/proj/emacs.git/

Comment on lines 301 to 314
# @FUNCTION: elisp-emacs-nativecomp
# @RETURN: exit status of Emacs
# @DESCRIPTION:
# Output whether currently active Emacs was built with nativecomp.

elisp-emacs-nativecomp() {
local cmd
if ! ver_test "${_ELISP_EMACS_VERSION}" -ge "${_EMACS_MAYBE_NATIVE_COMP}"; then
return 1
fi
cmd='(or (string-match-p "--with-nativecomp" system-configuration-options) (error "nativecomp not enabled"))'
return ${EMACS} ${EMACSFLAGS} -l "${cmd}"
}

# @FUNCTION: elisp-check-emacs-nativecomp
# @USAGE: [version]
# @DESCRIPTION:
# Test if the eselected Emacs version is at least the version of
# GNU Emacs specified in the NEED_EMACS variable, or die otherwise.

elisp-check-emacs-nativecomp() {
if [[ -z ${_ELISP_EMACS_NATIVE_COMP} ]]; then
elisp-emacs-nativecomp
_ELISP_EMACS_NATIVE_COMP=$?
fi
return ${_ELISP_EMACS_NATIVE_COMP}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if two functions are necessary here. Looks like a single elisp-emacs-nativecomp (but with the caching variable) would be sufficient?

OTOH, if you model it along the lines of elisp-emacs-version and elisp-emacs-check-version then the "check" function should die upon failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wondered about the naming here. Basically I wanted to check once whether emacs had support built, store that, and then reuse it instead of checking each time, but I don't think this should fail, because there aren't any packages that strictly require nativecomp in the same way that they might require a certain minimum version of emacs. Maybe just have elisp-emacs-nativecomp do the caching itself and get rid of elisp-emacs-check-nativecomp altogether?

Comment on lines 311 to 312
cmd='(or (string-match-p "--with-nativecomp" system-configuration-options) (error "nativecomp not enabled"))'
return ${EMACS} ${EMACSFLAGS} -l "${cmd}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That test looks complicated. How about this instead:

    [[ $(${EMACS} ${EMACSFLAGS} --eval "(princ (fboundp 'batch-native-compile))") == t ]]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is entirely safe given the checks that @AndreaCorallo added in emacs-mirror/emacs@6c108e4. But having now looked at that commit more closely it looks like calling comp-ensure-native-compiler would be the right thing to do here.

@tgbugs
Copy link
Contributor Author

tgbugs commented Aug 2, 2020

Testing it out in the emacs overlay seems like the right place since there is a good process for upstreaming from there. I can close this pr now, or continue to work on it here and then close it and send you the patches against the emacs overlay when it looks to be in reasonable shape.

@thesamesam
Copy link
Member

thesamesam commented Aug 25, 2020

Thanks for working on this, I'm quite excited to see iti in ::emacs at least. @trofi kindly made a fix to gcc's jit support in Gentoo after I asked about this.

For now, USE=jit is still masked on gcc but it should work fine if you unmask it.

The basic logic seems to be working at this point but the actual
compiling seems to encounter issues and does not fail correctly.
I also suspect that elisp-install may need to be made aware of the
fact that the eln folder may or may not be present and that it should
be installed in its entirety.

 * Emacs version: 28.0.50
 * Compiling GNU Emacs Elisp files ...                  [ ok ]
 * Native compiling GNU Emacs Elisp files ...
ld: cannot find crtbeginS.o: No such file or directory
ld: cannot find -lgcc
ld: cannot find -lgcc_s
libgccjit.so: error: error invoking gcc driver          [ ok ]

Closes: https://bugs.gentoo.org/735148

Signed-off-by: Tom Gillespie <tgbugs@gmail.org>
@tgbugs tgbugs force-pushed the elisp-common-nativecomp branch 2 times, most recently from d868bc2 to e552bdd Compare August 30, 2020 03:44
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-30 03:55 UTC
Newest commit scanned: d868bc2
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/77e83965d6/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-30 04:15 UTC
Newest commit scanned: e552bdd
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/dc57bdbf30/output.html

Use the elisp function comp-ensure-native-compiler to test whether
emacs supports native compilation. If the function name is void or
the native compiler is not enabled then emacs will exit with a
non-zero status. The exit status is checked once and cached in the
_EMACS_NATIVE_COMP variable to be returned by elisp-emacs-nativecomp
in all subsequent calls.

This has been tested against all >=emacs-23 in the tree.
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-30 04:35 UTC
Newest commit scanned: 123473e
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/afcff5de23/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-30 04:55 UTC
Newest commit scanned: b4f75f8
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/7b27162c22/output.html

@tgbugs
Copy link
Contributor Author

tgbugs commented Aug 30, 2020

I have cleaned up the test for the native compiler feature so that it matches best practices from upstream.

I am still encountering the ld issues, but I am using an install of gcc 9.3 where I manually unmasked the jit use flag that was built before the bug that @thesamesam points to was resolved.

There have been some changes to the way that the eln files are managed since I originally worked on this which might make this obsolete. Specifically, user eln files are now cached by default in ~/.emacs.d/eln-cache, so there are no longer issues with permissions to modify the eln directory used by the system.

However I suspect that admins would still like to be able to compile site-lisp once, instead of repeatedly for each user. However, in order to accomplish this some changes need to be made so that there is somewhere to put the site-lisp eln files which is not the system eln cache nor the user eln cache (see emacs-mirror/emacs@3224a44 for reference). In principle the system eln cache could be used for this, but that seems like that is not the correct solution. The addition of a 3rd eln cache to the comp-eln-load-path variable seems like it might be one potential solution. @AndreaCorallo do you have any thoughts on where to put the eln cache for site-lisp?

@AndreaCorallo
Copy link

Hi @tgbugs ,

could you describe what is the use case the current approach does not cover?

Currently the makefile is installing common eln files in .../libexec/emacs/28.0.50/x86_64-pc-linux-gnu/eln-cache/. This are in use by all emacsen.

Thanks

Andrea

@ulm
Copy link
Member

ulm commented Aug 30, 2020

Currently the makefile is installing common eln files in .../libexec/emacs/28.0.50/x86_64-pc-linux-gnu/eln-cache/. This are in use by all emacsen.

I am confused. It is in a directory with an explicit version in its path, so how can it be used "by all emacsen"?

@AndreaCorallo
Copy link

Hi @ulm,

yes this is in comp-eln-load-path. ATM the last entry in comp-eln-load-path is assumed to be the system cache directory and when Emacs is installed is fixed-up automatically to point there. Here http://akrl.sdf.org/gccemacs.html#org01229a9 a little more.

@ulm
Copy link
Member

ulm commented Aug 30, 2020

Then eln-cache is a misnomer for the system-wide directory. It is not a cache if it only contains files that are installed by the package.

(Or from a wider perspective, by the FHS a system-wide cache can never be in /usr but should be in /var/cache. And it would be populated at run time, not at install time.)

@AndreaCorallo
Copy link

You are welcome to propose a better name on emacs-devel, this is a development branch and we are currently making it up.

@tgbugs
Copy link
Contributor Author

tgbugs commented Aug 31, 2020

@AndreaCorallo I think that in terms of how the elisp files that are distributed as part of emacs itself is built everything is covered since the system eln files will not change without also changing the rest of the emacs distribution.

My primary question is about what to do about elisp files that are installed system wide in site-lisp. The use case is that I don't want each user to compile their own version and store the results in ~/.emacs.d/eln-cache, instead I want to compile the eln files once and store them in one place. The directory associated with the system install .../libexec/emacs/28.0.50/x86_64-pc-linux-gnu/eln-cache/ doesn't seem to make sense since it is essentially static and part of the emacs distribution (e.g. in a distro like debian it would be bundled in the .deb file). Basically can we add a common location in site-lisp that can act as a common place for elisp packages installed site wide to deposit eln files compiled before install?

The simplest solution I see would be to add /usr/share/emacs/site-lisp/eln-cache to comp-eln-load-path by default. This would be sufficient to support eln caches for multiple version of emacs since you have already worked out how to handle that internally.

(I'm ignoring the suggestion to rename eln-cache to something else for now, though I agree with @ulm it seems a bit confusing to call it a cache when in many cases the files being installed will always already be present at install time.)

@AndreaCorallo
Copy link

Mmh I see, so we are talking about elisp files installed system wide by the distro but not part of the vanilla emacs distro. And if I understand correctly you want to precompile the eln and distribute these directly correct?

In case yes, I think that installing these in like /usr/share/emacs/site-lisp/eln-cache and adding it to comp-eln-load-path would do the job. Not sure how the distro can modify the default of comp-eln-load-path tho. Or maybe we should add this /usr/share/emacs/site-lisp/eln-cache as second entry by default?

Note that as you mentioned eln files are per Emacs version so you'd need to have a different pkg for each.

(I'm ignoring the suggestion to rename eln-cache to something else for now, though I agree with @ulm it seems a bit confusing to call it a cache when in many cases the files being installed will always already be present at install time.)

We'll be changing it soon https://lists.gnu.org/archive/html/emacs-devel/2020-08/msg01036.html

@tgbugs
Copy link
Contributor Author

tgbugs commented Aug 31, 2020

Mmh I see, so we are talking about elisp files installed system wide by the distro but not part of the vanilla emacs distro. And if I understand correctly you want to precompile the eln and distribute these directly correct?

Yes

In case yes, I think that installing these in like /usr/share/emacs/site-lisp/eln-cache and adding it to comp-eln-load-path would do the job. Not sure how the distro can modify the default of comp-eln-load-path tho.

I took a quick peek and it seemed like it would require a patch or something similarly invasive. Thus my next point.

Or maybe we should add this /usr/share/emacs/site-lisp/eln-cache as second entry by default?

I think this is the best solution. It mirrors how emacs currently handles site-lisp, and would provide a natural convention that all distros could follow.

Note that as you mentioned eln files are per Emacs version so you'd need to have a different pkg for each.

Even on gentoo there is not support for having multiple major versions of emacs using their own elc files at the same time (thus the use of emacs-updater to recompile the elc files between major version). However I can imagine for binary distros that if it ever becomes an issue some enterprising maintainer will figure out how to run all the currently supported versions of emacs to get them to compile all the various eln-cache variants and put them in the same package, or they will ignore the issue and when the site-lisp is out of sync with emacs then it will fail over and each user will compile their own as is done now.

We'll be changing it soon https://lists.gnu.org/archive/html/emacs-devel/2020-08/msg01036.html

Woo!

@ulm
Copy link
Member

ulm commented Sep 1, 2020

Mmh I see, so we are talking about elisp files installed system wide by the distro but not part of the vanilla emacs distro. And if I understand correctly you want to precompile the eln and distribute these directly correct?

This would be the way to go for Gentoo. ("Distribute" in the sense of installing on users' system, since Gentoo as a source-based distro doesn't distribute binary packages.)

In case yes, I think that installing these in like /usr/share/emacs/site-lisp/eln-cache and adding it to comp-eln-load-path would do the job. Not sure how the distro can modify the default of comp-eln-load-path tho. Or maybe we should add this /usr/share/emacs/site-lisp/eln-cache as second entry by default?

Again, that directory is not a cache. These files would be installed by distros' package managers. Also they're architecture specific so they have no place in /usr/share.

My suggestion would be to install such files in /usr/lib{,64}/emacs/site-eln. (Alternatively, /usr/lib{,64}/emacs/site-lisp/eln would also be possible, but I don't see what the additional directory level would buy us.)

@ulm
Copy link
Member

ulm commented Sep 1, 2020

Or maybe we should add this /usr/share/emacs/site-lisp/eln-cache as second entry by default?

I think this is the best solution. It mirrors how emacs currently handles site-lisp, and would provide a natural convention that all distros could follow.

See above, please don't put arch specific files in share.

Even on gentoo there is not support for having multiple major versions of emacs using their own elc files at the same time (thus the use of emacs-updater to recompile the elc files between major version). [...]

Yeah, compiling for each Emacs version separately might be more correct. Then again, at least for the .elc files there haven't been many issues over the years. There was some fallout between Emacs versions 24.2 and 24.3 because of changes in the byte compiler, but apart from that .elc files byte-compiled with a previous Emacs version tend to work just fine.

@ulm ulm self-assigned this Sep 1, 2020
@ulm ulm added do not merge Please DO NOT MERGE this PR. It will not be assigned but it will be scanned by CI. and removed need assignment It was impossible to assign the PR correctly. Please assign it manually. labels Sep 1, 2020
@tgbugs
Copy link
Contributor Author

tgbugs commented Sep 1, 2020

See above, please don't put arch specific files in share.

Duh. Obvious now that you mention it, but it never consciously registered that share was arch independent.

@AndreaCorallo
Copy link

@tgbugs would be nice to have a bug filed as feature request for adding this folder to comp-eln-load-path so other can comment there if necessary and it can be referenced afterwards. Also that's basically my todolist :)

@tgbugs
Copy link
Contributor Author

tgbugs commented Sep 6, 2020

@AndreaCorallo will submit a feature request tomorrow since I managed to get the new location to install correctly today (via tgbugs/tgbugs-overlay@91b5e75). It works like a charm, though changing the search path at dump time confused me for a bit because the path only changes if the eln files are installed, which they weren't, because the installation process was deleting them. Knowing that you are using that as your todo list I have a few other little issues that I'll submit as well :D!

@AndreaCorallo
Copy link

Knowing that you are using that as your todo list I have a few other little issues that I'll submit as well :D!

Damn, apparently mine wasn't a smart move... :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug linked Bug/Closes found in footer, and cross-linked with the PR. do not merge Please DO NOT MERGE this PR. It will not be assigned but it will be scanned by CI.
Projects
None yet
6 participants