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

emacsPackages.cask: use melpaBuild #338010

Merged
merged 2 commits into from
Sep 3, 2024
Merged

emacsPackages.cask: use melpaBuild #338010

merged 2 commits into from
Sep 3, 2024

Conversation

AndersonTorres
Copy link
Member

Description of changes

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 August 28, 2024 19:49
@github-actions github-actions bot added the 6.topic: emacs Text editor label Aug 28, 2024
@AndersonTorres AndersonTorres force-pushed the upload-cask branch 2 times, most recently from 5c8fba2 to 5b752d2 Compare August 30, 2024 02:44
@AndersonTorres AndersonTorres force-pushed the upload-cask branch 2 times, most recently from e50412e to 5850c03 Compare August 30, 2024 22:22
@AndersonTorres AndersonTorres marked this pull request as draft August 30, 2024 22:22
jian-lin
jian-lin reviewed Aug 31, 2024
View reviewed changes
pkgs/applications/editors/emacs/elisp-packages/manual-packages/cask/package.nix Outdated
Comment on lines 30 to 31
pname = "cask-source";
inherit version;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this change. This is similar to NixOS/rfcs#171, right? I do not follow that RFC, but since it is closed, I guess we should not set pname and version here.

Copy link
Member Author

@AndersonTorres AndersonTorres Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I use this to track the various fetchers.
The experience of various hash mismatch in /nix/store/<hash>-source.drv is confusing and horrible.

TBH I have never read this RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it is only used for development? What about not including it in the final expression?

BTW, there is only one hash for this package. It would not be confusing if you build this package alone for development, right?

Copy link
Member Author

@AndersonTorres AndersonTorres Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As a package developer, sometimes I test various more or less unrelated things at the same time.
It is easier than creating a lot of dedicated branches and swapping among them.
Further, nixpkgs-review makes it easier to test various modifications at the same time.
(Obviously, since GitHub forces a merge-branch style of development, I split the modifications into dedicated branches before uploading them.)

Supposing nothing else goes wrong and I am dealing with only one fetcher, naming the fetcher is redundant indeed.

On the other hand, as a regular user I have bumped around hash mismatches when nixos-rebuild ing my local machine.
Not a serious problem, for sure - I just waited one day to try again.

Nonetheless the message is cryptic. By adding name it becomes a bit less cryptic.

Since it is common practice to add workarounds for many things too hard to fix in Nixpkgs, I feel justified in using this.
It is way easier to understand and accept when compared to (say) wafHook = waf.hook;...

Copy link
Contributor

@jian-lin jian-lin Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would say what you want is basically NixOS/rfcs#171. Pros and cons are discussed there. There must be a reason to not accept that RFC, right? In addition, I do not find any precedents in the repo in a quick search. So I slightly against this.

Not a blocker.

@AndersonTorres AndersonTorres force-pushed the upload-cask branch 3 times, most recently from fe75458 to 235bbf4 Compare September 2, 2024 02:58
jian-lin
jian-lin reviewed Sep 2, 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.

diff looks good now. I'll undraft it.

@jian-lin jian-lin marked this pull request as ready for review September 2, 2024 04:02
@AndersonTorres AndersonTorres force-pushed the upload-cask branch 2 times, most recently from e2bd4ee to ffad363 Compare September 2, 2024 16:49
jian-lin
jian-lin reviewed Sep 3, 2024
View reviewed changes
pkgs/applications/editors/emacs/elisp-packages/manual-packages/cask/package.nix
python3
]
++ (with emacs.pkgs; [
packageRequires = [
Copy link
Contributor

@jian-lin jian-lin Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Where do you find these dependencies? Maybe an update is needed.

It seems these listed packages are not referenced anywhere in the cask source code:

  • dash
  • ecukes
  • el-mock
  • ert-async
  • ert-runner
  • noflet
  • servant
  • shell-split-string

While, these referenced packages are not listed here:

  • commander
  • epl
  • shut-up

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaicr I take them from (older?) Cask file: https://github.com/cask/cask/blob/master/Cask

@AndersonTorres
emacsPackages.cask: use melpaBuild
78bf14b 
- use a patch that parameterizes lispdir
  - so that we can get rid of SRCDIR and other dependencies
- finalAttrs
- remove failing doCheck
- sei ignoreCompilationError to false
- meta.mainProgram
- meta.homepage points to GitHub
  - because the previous is outdated
- nixfmt-rfc-style
@jian-lin jian-lin merged commit beebab8 into NixOS:master Sep 3, 2024
8 of 9 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 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