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

fix default info path #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix default info path #64

wants to merge 2 commits into from

Conversation

izahn
Copy link
Contributor

@izahn izahn commented Jul 12, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Fixes #60

@conda-forge-linter
Copy link

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
Copy link
Contributor Author

izahn commented Jul 12, 2022

@conda-forge-admin please rerender

@izahn izahn changed the title fix defautl info path fix default info path Jul 12, 2022
Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

I left one comment. Feel free to ignore it. Otherwise nice work, thanks!


if [ "$(uname)" == "Linux" ]; then
mkdir -p $PREFIX/share/emacs/site-lisp
cat <<EOF > $PREFIX/share/emacs/site-lisp/site-start.el
Copy link

Choose a reason for hiding this comment

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

I feel a bit uneasy about this potentially overwriting an existing file. Would you mind adding a test for existence and/or use >> to append instead of overwriting in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is supposed to be created by site administrators, it is not included with emacs.

Copy link

Choose a reason for hiding this comment

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

Yes, but it's possible that it will be introduced in other parts of the packaging in the future, and adding another > seems like a trivial thing that wouldn't hurt much. But as I said, feel free to ignore. 👍

Copy link
Member

Choose a reason for hiding this comment

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

>> would do nothing. This is done at the build step. Whatever file is created in the build is what is shipped with the package. If this file is somehow included with other packages and we want to append it, that will have to be done in a post-install script. Although I'm unclear what other packages would ship files in $PREFIX/share/emacs.

Copy link

Choose a reason for hiding this comment

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

Alright, let's forget about this comment.

@asmeurer
Copy link
Member

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
Copy link
Member

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)

@izahn
Copy link
Contributor Author

izahn commented Jul 15, 2022

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
Copy link
Member

Where is the string actually stored in the package?

@zklaus
Copy link

zklaus commented Jul 15, 2022

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
Copy link
Contributor Author

izahn commented Jul 16, 2022

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
Copy link

zklaus commented Jul 18, 2022

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
Copy link
Contributor Author

izahn commented Jul 18, 2022

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.

@izahn izahn mentioned this pull request Jul 18, 2022
5 tasks
@zklaus
Copy link

zklaus commented Jul 18, 2022

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

@izahn
Copy link
Contributor Author

izahn commented Jul 18, 2022

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
Copy link
Contributor Author

izahn commented Jul 18, 2022

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

@izahn
Copy link
Contributor Author

izahn commented Jul 18, 2022

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
Copy link

zklaus commented Jul 19, 2022

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).

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

Successfully merging this pull request may close these issues.

Extra nulls in path string with 28.1 Linux package
4 participants