conda-forge / emacs-feedstock

A conda-smithy repository for emacs.
BSD 3-Clause "New" or "Revised" License
6 stars 16 forks source link

fix default info path #64

Open izahn opened 2 years ago

izahn commented 2 years ago

Checklist

Fixes https://github.com/conda-forge/emacs-feedstock/issues/60

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

izahn commented 2 years ago

@conda-forge-admin please rerender

asmeurer commented 2 years ago

We don't know why this happens in the first place? Presumably wherever this path is stored is using a fixed length string instead of a NULL-terminated string.

At any rate we should include a comment to indicate that we should check if this is still needed whenever new versions are released.

asmeurer commented 2 years ago

Oh I didn't see https://github.com/conda-forge/emacs-feedstock/issues/60#issuecomment-1181538684. So a more complete fix might require somehow making the conda-build binary string patching do the right thing for lisp strings, or patching make_string for the build. (I do wonder if there aren't other instances of this somewhere)

izahn commented 2 years ago

Oh I didn't see #60 (comment). So a more complete fix might require somehow making the conda-build binary string patching do the right thing for lisp strings, or patching make_string for the build. (I do wonder if there aren't other instances of this somewhere)

Yes, but I think perhaps the problem is that make_string records the length as well as the value. Then conda build patches the string, but emacs still thinks the length is the original length. I'm not super confident in this analysis, but if this is what happens then it's not clear to me how to fix it properly. Hence the after-the-fact fixup here instead of a proper fix at the source of the problem.

asmeurer commented 2 years ago

Where is the string actually stored in the package?

zklaus commented 2 years ago

Unfortunately, this PR does not seem to fix the problem. It improves the situation because it fixes the path earlier than if you put it in your init.el, but I saw that it still produces errors on the command line that don't happen with the fix in early-init.el.

The other thing that puzzles me is that this make_string stuff should happen at runtime, right? So at that point, the path should already be replaced and strlen should correctly detect the shorter length of the actual path. I don't know why that doesn't happen. Either the code is executed during build time to construct some config input file that is then later installed, or perhaps strlen is optimized away because the passed string is a character constant?

izahn commented 2 years ago

Unfortunately, this PR does not seem to fix the problem. It improves the situation because it fixes the path earlier than if you put it in your init.el, but I saw that it still produces errors on the command line that don't happen with the fix in early-init.el.

Can you provide reproduction steps for this?

The other thing that puzzles me is that this make_string stuff should happen at runtime, right?

I'm not really sure, I assumed it happened at build time but I don't know why I thought that.

zklaus commented 2 years ago

Unfortunately, this PR does not seem to fix the problem. It improves the situation because it fixes the path earlier than if you put it in your init.el, but I saw that it still produces errors on the command line that don't happen with the fix in early-init.el.

Can you provide reproduction steps for this?

I simply put the site-init.el into my current installation. Then starting emacs on a terminal leads to 36 copies of

Wrong type argument: filenamep, "$CONDA_PREFIX/emacs/share/info/"
izahn commented 2 years ago

I simply put the site-init.el into my current installation. Then starting emacs on a terminal leads to 36 copies of

Can you test by installing from the izahn channel? It works for me.

zklaus commented 2 years ago

I tried installing from izahn; it gave the same result.

izahn commented 2 years ago

I tried installing from izahn; it gave the same result.

I definitely can't reproduce that. Does it happen with emacs -q? If not, can you bisect your init to find out what causes it? Maybe you have something left over in early-init.el.

izahn commented 2 years ago

Oh, there is a bug though, hang on while I fix it.

izahn commented 2 years ago

Well, after digging into a bit more I do see the problem you're talking about @zklaus . Indeed it seems that site-start.el is loaded too late in the init sequence to avoid problems. Bummer!

In my tests it does seem that setting INFOPATH as documented in https://www.gnu.org/software/emacs/manual/html_node/info/Emacs-Info-Variables.html avoids the problem. We can set this as documented at https://conda-forge.org/docs/maintainer/adding_pkgs.html#activate-scripts I guess, though that mechanism is not foolproof.

zklaus commented 2 years ago

Here is another idea: We could patch this snippet directly into lisp/startup.el, for example at the beginning of (defun normal-top-level ()) (ca l. 530).