Skip to content

buildCrystalPackage: add copyShardDeps flag#289266

Merged
peterhoeg merged 1 commit intoNixOS:masterfrom
sund3RRR:build-crystal-package-fix
Apr 19, 2024
Merged

buildCrystalPackage: add copyShardDeps flag#289266
peterhoeg merged 1 commit intoNixOS:masterfrom
sund3RRR:build-crystal-package-fix

Conversation

@sund3RRR
Copy link
Contributor

@sund3RRR sund3RRR commented Feb 16, 2024

Description of changes

Added the ability to copy dependencies declared in shards.nix instead of creating symbolic links to dependencies. By default, the copyShardDeps flag is set to false, so this change should not break other packages in any way.

This change will have an extremely positive effect on the packaging of crystal packages, since the state of crystal dependencies at the time of download is not final and may change for a number of things:

  1. shards.yml may contain a postInstall script that wants to write to the shard's directory (which will lead to build error, because all shards are in /nix/store and they are read only)
  2. gi-crystal cannot work in nix, because it generates bindings in its folder, so by creating a link to gi-crystal in /nix/store, we actually prohibit gi-crystal from working and it becomes useless. I patched it and changed the logic of it, so it works at the moment, however, each new version of gi-crystal breaks packages which depends on it, since the version of gi-crystal must match the version from shard.lock
  3. crystal macros can be executed at the time of program compilation and write something to the shard's directory. (I ran into this problem in blake3.cr )

With this change, the logic of buildCrystalPackage does not change in any way, but an important setting is added, by adding just one flag, you can create an environment closer to the standard environment and simplify packaging.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@sund3RRR sund3RRR mentioned this pull request Feb 16, 2024
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Feb 16, 2024
@sund3RRR sund3RRR force-pushed the build-crystal-package-fix branch from 3b00817 to 4eb527b Compare February 18, 2024 10:54
@sund3RRR sund3RRR changed the title buildCrystalPackage: add copyShardsDeps flag buildCrystalPackage: add copyShardDeps flag Feb 18, 2024
@sund3RRR
Copy link
Contributor Author

@SuperSandro2000

@sund3RRR sund3RRR added 6.topic: crystal Programming language - https://crystal-lang.org/ 0.kind: enhancement Add something new or improve an existing system. awaiting_reviewer labels Feb 22, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3535

@peterhoeg
Copy link
Member

Thanks a lot for doing this - I haven't been able to afford the time for a proper review that this deserves, but will take a look in the near future. Please also note that we have a Google Summer of Code project idea related to crystal.

@peterhoeg peterhoeg self-assigned this Feb 28, 2024
@sund3RRR sund3RRR mentioned this pull request Mar 21, 2024
13 tasks
@sund3RRR
Copy link
Contributor Author

sund3RRR commented Apr 3, 2024

Hi, guys. I want to fix all packages related to this change before the new NixOS 24.05 release. Can someone review this PR, please?

@sund3RRR
Copy link
Contributor Author

sund3RRR commented Apr 3, 2024

@SuperSandro2000 could you review my PR please?

@sund3RRR sund3RRR force-pushed the build-crystal-package-fix branch from 4eb527b to f2b185f Compare April 3, 2024 17:59
@sund3RRR sund3RRR requested a review from SuperSandro2000 April 3, 2024 18:00
@sund3RRR
Copy link
Contributor Author

sund3RRR commented Apr 5, 2024

@SuperSandro2000

Copy link
Member

@donovanglover donovanglover left a comment

Choose a reason for hiding this comment

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

LGTM

@Artturin
Copy link
Member

Artturin commented Apr 8, 2024

Please add the description to the commit message then 👍

@sund3RRR
Copy link
Contributor Author

sund3RRR commented Apr 9, 2024

Please add the description to the commit message then 👍

What description should be added to the commit?

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 12, 2024
@peterhoeg
Copy link
Member

Looks good, thanks @sund3RRR

@peterhoeg peterhoeg merged commit 9c1f3b3 into NixOS:master Apr 19, 2024
@Artturin
Copy link
Member

Artturin commented Apr 19, 2024

Please add the description to the commit message then 👍

What description should be added to the commit?

The pull request description, it's good to have it in the commit because github can at any moment remove PRs and has done so many times before.

It's good for writing docs too https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/crystal.section.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: enhancement Add something new or improve an existing system. 6.topic: crystal Programming language - https://crystal-lang.org/ 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants