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.

Sign up for GitHub

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

Jump to bottom

lspce: 1.0.0-unstable-2024-02-03 -> 1.1.0-unstable-2024-07-13 #326981

Merged
merged 2 commits into from
Jul 19, 2024
Merged

lspce: 1.0.0-unstable-2024-02-03 -> 1.1.0-unstable-2024-07-13 #326981

merged 2 commits into from
Jul 19, 2024

Conversation

AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented Jul 14, 2024 •
edited by jian-lin
Loading

Description of changes

Fixing an Emacs package.

related: #278925

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@AndersonTorres AndersonTorres marked this pull request as ready for review July 14, 2024 01:57
@github-actions github-actions bot added the 6.topic: emacs Text editor label Jul 14, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jul 14, 2024
jian-lin
jian-lin reviewed Jul 14, 2024
View reviewed changes
Copy link
Contributor

@jian-lin jian-lin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fixing cargoHash is enough to fix the build.

The old way of packaging makes more sense to me because lspce is an Emacs package which uses a dynamic module.

CI show another cargoHash error. Weird.

@AndersonTorres
Copy link
Member Author

The old way of packaging makes more sense to me because lspce is an Emacs package which uses a dynamic module.

Hum, in a certain sense I agree. This package is not really useful outside Emacs.

However, the hardest work is made by Rust toolchain, while the Emacs part looks like a wrapper.
Indeed the modules can be built without any interference from Emacs ecosystem.
Further, technically the Emacs part depends on the Rust part.

Because of it I reworked the package this way: a Rust package with an Emacs companion package as passthru.

@jian-lin
Copy link
Contributor

Indeed the modules can be built without any interference from Emacs ecosystem.
Further, technically the Emacs part depends on the Rust part.

Every Emacs package using a dynamic module depends on emacs-module.h of Emacs to build. lspce builds without explicit need of emacs-module.h because its core dependency, the emacs crate, vendors emacs-module.h.

@jian-lin
Copy link
Contributor

The version change in #316332 causes the change of cargoHash. More info: #112763 (comment)

jian-lin
jian-lin previously requested changes Jul 17, 2024
View reviewed changes
pkgs/by-name/ls/lspce/package.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member Author

Apologies for the delay, I was dealing with some orphans and corpses.

I took a middle approach:

  • The lspce-module has its own file;
  • The elisp code calls this module.

jian-lin
jian-lin requested changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@jian-lin jian-lin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lspce.el fails to compile.

My fix (with some minor improvements such as adding an update script) is available here: https://github.com/atorres1985-contrib/nixpkgs/compare/6837f904c5fad033ff35074903953c0de2a727bf...linj-fork:nixpkgs:tmp-pr/326981

If you are busy, I am willing to create a new PR because it seems that I cannot push to this specific PR for some reason.

@jian-lin
Copy link
Contributor

Ideally, the bump and the refactor should be two separate PRs.

AndersonTorres and others added 2 commits July 19, 2024 10:13
@AndersonTorres @jian-lin
emacsPackages.lspce: rework
8c08052 
- detach Rust module to module.nix
- trivialBuild -> melpaBuild
- updateScript
- adopt (by AndersonTorres)

Co-authored-by: Lin Jian <[email protected]>
@AndersonTorres
emacsPackages.lspce: 1.0.0-unstable-2024-02-03 -> 1.1.0-unstable-2024…
92ebcc1 
…-07-13
jian-lin
jian-lin approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

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

LGTM

@jian-lin jian-lin merged commit 619ef5d into NixOS:master Jul 19, 2024
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jian-lin jian-lin jian-lin approved these changes

@adisbladis adisbladis Awaiting requested review from adisbladis

Assignees
No one assigned
Labels
6.topic: emacs Text editor 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndersonTorres @jian-lin