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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gi-crystal: remove package #305484

Closed
wants to merge 1 commit into from
Closed

Conversation

sund3RRR
Copy link
Contributor

Description of changes

Remove gi-crystal package because it is useless now as we have copyShardDeps flag in buildCrystalPackage func, so gi-crystal works now without any patches and packaging.
Since both packages that depend on gi-crystal are broken, we can remove gi-crystal now.
But it is better to wait until collision and rtfm are merged.

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.

@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/1016/127

@UlyssesZh
Copy link
Contributor

I think it may be better to add a useGiCrystal argument to crystal.buildCrystalPackage, and enabling that argument will

  • toggle on copyShardDeps by default,
  • add gobject-introspection to nativeBuildInputs, and
  • add the following to build phase:
mkdir bin
cd lib/gi-crystal
shards build -Dpreview_mt --release --no-debug
cd ../..
cp lib/gi-crystal/bin/gi-crystal bin

This may reduce some common codes. The only thing that needs to be patched is then removing shards install commands in the Makefile.

@UlyssesZh
Copy link
Contributor

Remove gi-crystal package because it is useless now as we have copyShardDeps flag in buildCrystalPackage func, so gi-crystal works now without any patches and packaging.

I don't know whether gi-crystal should be considered useless. A benefit of having gi-crystal is to make gi-crystal a separate derivation from the package (say collision) that uses gi-crystal, so we don't have to compile gi-crystal every time we build collision.

@sund3RRR
Copy link
Contributor Author

I think it may be better to add a useGiCrystal argument to crystal.buildCrystalPackage, and enabling that argument will

  • toggle on copyShardDeps by default,
  • add gobject-introspection to nativeBuildInputs, and
  • add the following to build phase:
mkdir bin
cd lib/gi-crystal
shards build -Dpreview_mt --release --no-debug
cd ../..
cp lib/gi-crystal/bin/gi-crystal bin

This may reduce some common codes. The only thing that needs to be patched is then removing shards install commands in the Makefile.

These are good thoughts and I like it. But I think there's something wrong with the crystal on the Nix.
You know, like... we need to set the shards.nix file, then set copyShardDeps = true, then set useGiCrystal = true;, and then add a couple of patches, because nothing works with Crystal on nix.

While the "I use Arch btw" guys just do shards install && shards build and push their package to the AUR, we need...

  1. Download Collision, try to package...
  2. Realize that something is wrong, because it just won鈥檛 work
  3. Take a couple of days to understand why
  4. Got it! Take a couple of days to figure out how to fix this...
  5. gi-crystal package with some patches!
  6. Understand that this is a bad idea because every package that depends on it must have its own gi
    -crystal because the version must match the version in shard.yml.
  7. Take a couple of days to figure out how to fix this...
  8. Make a PR to the main buildCrystalPackage function
  9. Wait a couple of months until someone checks and merges the PR
  10. FINALLY the collision package is correct, but you have to add this shit:
mkdir bin
cd lib/gi-crystal
building shards -Dpreview_mt --release --no-debug
cd ../..
cp lib/gi-crystal/bin/gi-crystal bin

because you still can't run shards install and you need to build it manually :)))))))

I've spent so much time trying to package crystal applications because we have over-engineering for... over-engineering!

While the Arch Linux guys have a human environment on their system, we create a declarative shards.nix file on top of the declarative shard.yml and shard.lock LOCK FILE. We have a lock file, why do we need another lock????

I feel like a gun without bullets. I have the dependencies, but I can't use them because gi-crystal can't run from /nix/store, and blake3.cr doesn't build because Crystal2nix doesn't run scripts after installation.

While people use the default human-like environment, we can't use shards install because it is less reproducible than nix.

You know, it gets really crazy when we talk about reproducibility. We store deps into /nix/store, but we need the copyShardDeps flag to restore the normal project structure, because without it it doesn't work. Why do we use this shit if we still need to restore the project structure and copy it? Why don't we just use shards install during the download phase????

Because we don't like non-reproducible builds. But do we love so many hacks and patches? Do we really need all this over-engineering? It's not normal for you to need so much time just to run a program.

Going back to the useGiCrystal flag, I think it's better to create another builder called buildGiCrystalApplication or something like that.
Like python with buildPythonPackage and buildPythonApplication.
Add a preBuild hook to create and run gi-crystal.

But I have to do so much work for this, so... I don't know, it would be great if we just run shards install...

@UlyssesZh
Copy link
Contributor

This situation with Crystal packages is actually the result of multiple factors, not just nix. The lib dir shouldn't need write access by design; shards should not manage build time dependencies by design, either.

I am actually surprised that, when you packaged gi-crystal in the first place (#243914), you didn't propose the changes to the upstream (or maybe you did?). Making gi-crystal a standalone build tool instead of a dependency managed by shards is so much better.

Having buildGiCrystalApplication is fine. I just thought adding a flag to buildCrystalPackage would be less work.

@sund3RRR
Copy link
Contributor Author

I think having gi-crystal in nixpkgs is a bad idea because this thing not backwards compatible. Means gi-crystal 0.23.0 may not work if the package expects gi-crystal 0.22.0.

Another reason is that it is now a patched gi-crystal with changed logic. We have a problem: we need to support our patches, which Hugo won't support.

If Hugo decides to generate bindings outside of the lib/ folder, we'll be fine with it. But we shouldn't apply these hard patches, our goal in my opinion is just to pack it.

I don't want to add another flag to buildCrystalPackage just because it will take a long time waiting for someone to review and merge it. I want to fix all packages related to this issue as quickly as possible because the release of NixOS 24.05 Uakari is approaching.

@UlyssesZh
Copy link
Contributor

I don't want to add another flag to buildCrystalPackage just because it will take a long time waiting for someone to review and merge it. I want to fix all packages related to this issue as quickly as possible because the release of NixOS 24.05 Uakari is approaching.

I think it should be fine to just do it in this PR.

@sund3RRR
Copy link
Contributor Author

@ofborg eval

@sund3RRR
Copy link
Contributor Author

@GrahamcOfBorg eval

@sund3RRR sund3RRR closed this May 23, 2024
@UlyssesZh
Copy link
Contributor

Why close?

@sund3RRR
Copy link
Contributor Author

Cause lusasew made a cool update script for that package. I don't want to make another flag or builder for buildCrystalPackage func. But maybe someone wants to use gi-crystal as a nix package for development. So I decided to leave this package in nixpkgs.

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

Successfully merging this pull request may close these issues.

None yet

4 participants