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

Test if packages are loadable with the wrapper #170

Merged
merged 4 commits into from
Jun 3, 2023

Conversation

akirak
Copy link
Collaborator

@akirak akirak commented May 31, 2023

The test which I added three years ago only builds emacsWithPackages wrapper with a few packages. Since then, there have been several changes made in the upstream nixpkgs repository. Unfortunately, the wrapper no longer correctly starts if it is combined with an Emacs derivation from nix-emacs-ci, which was not detected by the test.

This PR extends the test to check if the wrapper correctly starts and loads packages.

@akirak
Copy link
Collaborator Author

akirak commented May 31, 2023

@purcell site-start.el is required by the wrapper, and it is supposed to be created here. Without the file, Emacs built using emacs.pkgs.withPackages fails to start, but creating an empty file seems to fix the startup error. What do you think?

@purcell
Copy link
Owner

purcell commented May 31, 2023

Yeah, upstream in nixpkgs the site-start.el looks like this: https://github.com/NixOS/nixpkgs/blob/342d73c9355613f36304a19d272d75ca86502d14/pkgs/applications/editors/emacs/site-start.el, but I'm not worried about missing any of that content, so an empty site-start.el sounds viable.

@akirak akirak changed the title Test if packages are loadable using the low-level API Test if packages are loadable with the wrapper May 31, 2023
@akirak
Copy link
Collaborator Author

akirak commented Jun 1, 2023

Rebased onto master to trigger rebuilds with the latest nixpkgs.

@purcell
Copy link
Owner

purcell commented Jun 2, 2023

Is the extra treeSitter flag var necessary? I thought the previous code was doing the right thing, ie. emacs versions that supported tree-sitter would automatically detect and enable it.

@akirak
Copy link
Collaborator Author

akirak commented Jun 2, 2023

Is the extra treeSitter flag var necessary?

It is only intended to enable the tree-sitter integration of the wrapper. Alternatively, the user of nix-emacs-ci could set the attribute by overriding the attribute as follows (overriding passthru doesn't trigger rebuilds, so the binary cache would be still effective):

 emacs.overrideAttrs (_: {
    passthru.treeSitter = true;
  })

To use the feature in a project like #168, either this library or the user must pay the cost. Although nix-emacs-ci doesn't necessarily have to work around every possible feature of the wrapper, the empty site-start.el is already part of this PR, so one possible way would be to sensibly support common use cases through this library.

It's possible to handle the problem on the user side, so I will revert the commit if you don't prefer the additional complexity.

@akirak
Copy link
Collaborator Author

akirak commented Jun 2, 2023

I have removed the commit to set treeSitter from this branch and made it a separate PR (#172).

@purcell purcell merged commit d2b5f7b into purcell:master Jun 3, 2023
@purcell
Copy link
Owner

purcell commented Jun 3, 2023

Great, thanks!

@purcell
Copy link
Owner

purcell commented Jun 3, 2023

It is only intended to enable the tree-sitter integration of the wrapper.

Oh — that makes sense.

@akirak akirak deleted the loadability branch June 3, 2023 08:58
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.

2 participants