Skip to content

collision: 3.6.0 -> 3.8.0#282286

Closed
sund3RRR wants to merge 1 commit intoNixOS:masterfrom
sund3RRR:collision-3.7.0
Closed

collision: 3.6.0 -> 3.8.0#282286
sund3RRR wants to merge 1 commit intoNixOS:masterfrom
sund3RRR:collision-3.7.0

Conversation

@sund3RRR
Copy link
Contributor

@sund3RRR sund3RRR commented Jan 20, 2024

Description of changes

Update version and install nautilus-python extension.

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.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jan 20, 2024
Copy link
Contributor

@viraptor viraptor left a comment

Choose a reason for hiding this comment

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

See the failures on aarch64: https://github.com/NixOS/nixpkgs/pull/282286/checks?check_run_id=20684676737
The blake lib seems to be missing.
(Previous approval accidental :( )

@sund3RRR
Copy link
Contributor Author

See the failures on aarch64: https://github.com/NixOS/nixpkgs/pull/282286/checks?check_run_id=20684676737 The blake lib seems to be missing. (Previous approval accidental :( )

This is a really strange issue, I don't know how this successfully builds and works on x86_64 and fails on aarch64.
I can't compile it on my aarch64 machine because there doesn't seem to be a libblake3 in nixpkgs.

@sund3RRR sund3RRR requested a review from viraptor January 20, 2024 20:33
@sund3RRR
Copy link
Contributor Author

Ok, I figured out why it doesn't work on aarch64. Crystal's blake3 dependency builds a static library libblake3.a, but default Makefile target builds libarary for x86_64 regardless of the machine architecture. So, libblake3.a on my aarch64 machine contains these files:

ar -t /nix/store/kh4kh9qczdw0lpmrm9pjg499bqh6iq5p-blake3.cr/blake3c/libblake3.a
blake3.o
blake3_dispatch.o
blake3_portable.o
blake3_sse2.o
blake3_sse41.o
blake3_avx2.o
blake3_avx512.o
blake3_sse2_x86-64_unix.S
blake3_sse41_x86-64_unix.S
blake3_avx2_x86-64_unix.S
blake3_avx512_x86-64_unix.S

There is a Makefile target for aarch64:

blake3_neon.o: blake3_neon.c
	$(CC) $(CFLAGS) $(EXTRAFLAGS) -c $^ -o $@

But I don't know, how to redefine this target. I can manually replace blake3.cr dependency for aarch64 with the correct build in the collision derivation.

@GeopJr
Copy link
Member

GeopJr commented Jan 21, 2024

FWIW, the flatpak solution was to just pass env vars so it builds the neon target only on aarch64 https://github.com/flathub/dev.geopjr.Collision/blob/master/dev.geopjr.Collision.yaml#L43-L47

@sund3RRR
Copy link
Contributor Author

FWIW, the flatpak solution was to just pass env vars so it builds the neon target only on aarch64 https://github.com/flathub/dev.geopjr.Collision/blob/master/dev.geopjr.Collision.yaml#L43-L47

Hi, GeopJr. Thank you for your help. Unfortunately, crystal2nix, which is used for collecting collision dependencies doesn't run post_install targets in shard.yaml from blake3.cr. This is because in nix all packages are isolated from each other and in order to execute custom package build scripts, you will have to create an additional build environment in which gcc would be.
In other words, whatever I write in the blake3.cr's Makefile, it won't run. It works on x86_64 because your repository contains prebuild libblake3.a.
The only solution I found is to write a custom build script and package blake3.cr together with collision.
Something like this:

    if [ "$ARCH" == "x86_64" ]; then
        make -f ../build/Makefile
    elif [ "$ARCH" == "aarch64" ]; then
        BLAKE3_USE_NEON=1 make -f ../build/Makefile blake3_neon.o
    else
        echo "Unsupported architecture: $ARCH"
        cd ..
        exit 1
    fi

@sund3RRR
Copy link
Contributor Author

It works now, thanks to @GeopJr for advice with the environment variables.

@sund3RRR
Copy link
Contributor Author

@ofborg eval

@GeopJr
Copy link
Member

GeopJr commented Jan 22, 2024

Unfortunately, crystal2nix, which is used for collecting collision dependencies doesn't run post_install targets in shard.yaml from blake3.cr.

In other words, whatever I write in the blake3.cr's Makefile, it won't run. It works on x86_64 because your repository contains prebuild libblake3.a.

Nice catch thanks!

I don't like that it relies on a postinstall script either :/
I do have an idea, maybe I could check with crystal macros if blake3c is available and only compile it if it isn't without relying on postinstall. I'll need some time to test it properly (if it can be done at all), if possible please postpone this update until 3.7.1! Sorry for the inconvenience 🙇

@GeopJr
Copy link
Member

GeopJr commented Jan 24, 2024

Released 3.7.1, it should now have fixed the issues! Please make blake.cr a regular shard again. At compile time it only requires a C compiler to be available for blake3c. It should also build the correct target for aarch64 automatically

@sund3RRR sund3RRR changed the title collision: 3.6.0 -> 3.7.0 collision: 3.6.0 -> 3.7.1 Jan 26, 2024
@sund3RRR
Copy link
Contributor Author

I spent so much time building, but nothing worked. blake3-cr cannot run on nix as a shard due to the read-only filesystem, so blake3c will not compile due to a write permission error. I placed this package as a regular dependency without a symlink, but for some reason Crystal on Nix can't build the original C library, I spent two days trying to figure out why, but to no avail. So I still build blake3-cr in another derivation, I don't see any other way out

@GeopJr
Copy link
Member

GeopJr commented Jan 26, 2024

I can't setup a nix environment at the moment so I'll try to describe the build steps if it helps at all, otherwise do what you believe it's best!

When the crystal compiler builds collision, in the preprocessing phase, it will check if lib/blake3/blake3c/libblake3.a exists. If it doesn't it will run the compile_blake3.cr script that will prepare the right commands to build blake3c for the current arch and execute them.

From my understanding based on your last comment, the problem is here, lib/blake3/ is read-only. Not sure why that is as I'm sure other shards in other crystal programs might also need to modify their files with macros but 🤷

Some other stuff might worth looking into:

  • crystal/build-package.nix includes some additional options, not sure if it's possible to make it run crystal run blake3/src/compile_blake3.cr (or whatever its path is there) so it builds blake3c in advance
  • If in the future you find a better way to provide libblake3.a you can skip blake3.cr building it by setting BLAKE3_CR_DO_NOT_BUILD=1

No idea if anything else can be done from my side and don't really want to waste more of your time with more patch releases so I'd say go ahead and package it however you see fit and when I setup nix again I'll revisit it! Thanks again!

@sund3RRR
Copy link
Contributor Author

I can't setup a nix environment at the moment so I'll try to describe the build steps if it helps at all, otherwise do what you believe it's best!

Thank you for your help, GeopJr! It's 100% a nix problem. This is not feedback about your update, but simply a report on the work done.

From my understanding based on your last comment, the problem is here, lib/blake3/ is read-only. Not sure why that is as I'm sure other shards in other crystal programs might also need to modify their files with macros but 🤷

I think it's all crystal2nix fault. It installs collision dependencies in /nix/store and creates symlinks to these folders. This means that during the build phase we do not have permissions to change dependencies folders. Macros and post_build in shards will not work.

Some other stuff might worth looking into:
crystal/build-package.nix includes some additional options, not sure if it's possible to make it run crystal run blake3/src/compile_blake3.cr (or whatever its path is there) so it builds blake3c in advance
If in the future you find a better way to provide libblake3.a you can skip blake3.cr building it by setting BLAKE3_CR_DO_NOT_BUILD=1

The main problem is that I can build blake3-cr in other derivation, but when I run the collision build, it can’t build blake3c. I think that nix set some cflags/ldflags/etc, which prevent successful compilation and libblake3.a does not appear.
I set up a test environment on ubuntu and it worked, you did a good job, the problem is not related to the package itself.

@GeopJr
Copy link
Member

GeopJr commented Jan 26, 2024

I think that nix set some cflags/ldflags/etc, which prevent successful compilation and libblake3.a does not appear.

The script does respect them https://github.com/GeopJr/blake3.cr/blob/main/src/compile_blake3.cr#L22-L23 maybe it shouldn't? if nix allows you to, you could try to unset them or set them to the ones from the script

This was referenced Feb 4, 2024
@sund3RRR
Copy link
Contributor Author

In general, I figured it out. All the problems were due to symlinks. For some reason, crystal did not run the libblake3.a build at all. When I manually copied the entire shard, the library still did not build, it turned out that all file permissions also applied when copying and crystal simply did not have the rights to write at least something to the lib/blake3.cr folder, since all files remained read-only. I did a PR with changes to buildCrystalPackage, so now adding just one copyShardsDeps flag solves all problems with both gi-crystal and blake3.cr since the building environment in nix will now be practically the same as ubuntu's env. Thanks to GeopJr for the advice and for the help with the packaging!

I will make changes to the current PR as soon as crystal's PR is merged.

@sund3RRR
Copy link
Contributor Author

Damn, I broke the branch I think. How to fix it?

@sund3RRR sund3RRR closed this Apr 20, 2024
@sund3RRR sund3RRR reopened this Apr 20, 2024
@sund3RRR sund3RRR force-pushed the collision-3.7.0 branch 4 times, most recently from 715c913 to c99aed4 Compare April 20, 2024 06:47
@sund3RRR
Copy link
Contributor Author

Okay, that's fixed now.
Collision updated to 3.7.1. Unfortunately, I can't upgrade to 3.8.0 version. due to Adw::AboutDialog, which was added in libadwaita 1.5, but the current version in nixpkgs is 1.4.4.
Added nautilus extension so you can install gnome.nautilus-python system-wide and Check hashes will appear automatically.

@sund3RRR sund3RRR mentioned this pull request Apr 20, 2024
13 tasks
@sund3RRR
Copy link
Contributor Author

@viraptor

Copy link
Contributor

@viraptor viraptor left a comment

Choose a reason for hiding this comment

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

I'm not near a machine that can test it, but it looks good 👍

@sund3RRR
Copy link
Contributor Author

I forget to remove gi-crystal from inputs

@sund3RRR sund3RRR requested a review from viraptor April 23, 2024 10:14
@sund3RRR sund3RRR changed the title collision: 3.6.0 -> 3.7.1 collision: 3.6.0 -> 3.8.0 May 16, 2024
@sund3RRR
Copy link
Contributor Author

@viraptor could you please review the changes related to the new collision version? Collision still broken in nixpkgs, so we want to fix it before 24.05 released.

@sund3RRR sund3RRR mentioned this pull request May 17, 2024
13 tasks
@sund3RRR sund3RRR closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants