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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dinoscore: init at 0.4.0 #223416

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

piegamesde
Copy link
Member

Description of changes

This is marked as draft because I plan on releasing 0.4.1 with bug fixes soon.

Also I was sick of having to search for an appropriate location for the package, so I simply took pkgs/unit/di instead as suggested in NixOS/rfcs#140 for now :)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
    • Python-requiring features not working, TODO
  • 23.05 Release Notes (or backporting 22.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.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Using pkgs/unit from RFC 140 is not okay! For one, the RFC isn't accepted yet, meaning it might still change considerably or not even get accepted at all. And for another, a lot of thought went into trying to establish pkgs/unit as a cleaner foundation, with CI checks to ensure units don't access files outside their unit, autocalling to remove the burden to edit all-packages.nix, the limitation that you can't use them for non-packages for now, the establishment of package.nix files, lowecasing of shards to avoid problems on Darwin, etc. These things have a point, they're not just extras.

And I find it rather disrespectful for you to just go ahead and open this PR without even pinging anybody from the RFC. The first PR to introduce a new convention sets a precedent, meaning that if this were merged, we can expect a lot of others to follow, which in turn causes a series of problems. And once people are used to something, that thing becomes the convention, even if it has functional problems.

In fact this is also exactly why we're in such a deep mess with Flakes right now, because Eelco just ended up pushing Flakes as experimental when the RFC struggled. People started using it despite being experimental, and now we're having a really hard time changing it. We cannot let that happen here.

If you like the idea of the RFC, then the best way to make it go faster is to help out with it, we're still looking for a shepherd after all, and until we find one we can't proceed with it.

@piegamesde
Copy link
Member Author

This is not intended to be merged as-is, and I'm sorry for not making that more clear beforehand. Although currently I'm thinking of leaving this open until after RFC 140 has been merged … (provided that this happens within the next year or so)

@infinisil
Copy link
Member

I see, no problem then, sorry for lashing out a bit!

@symphorien
Copy link
Member

this is missing wrapGAppsHook: it crashes for me when opening the file dialog

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-04-03-nixpkgs-architecture-team-meeting-35/26950/1

@piegamesde
Copy link
Member Author

piegamesde commented Apr 5, 2023

I can't get the Python part to work properly, and I'm out of ideas :/

In short:

  • DiNoScore depends on some Python library via inline Python code and pyo3
  • Since pyo3 makes the binary link against libpython3.so instead of calling the python binary, the usual python3.withPackages does not work to get the dependencies. (As I understand, withPackages only wraps the PATH of the binary, so it has no effect when linking against libpython3 instead)
  • Simply adding the dependency from python3Packages into the buildInputs (or nativeBuildInputs, does not matter for now) helps in so far that it makes the tests go through (some of the tests also require that Python library). But in the final binary, outside of the build environment, it still does not work
  • To reproduce, run dinoscore-editor, click "Import score", and select any PDF file. It should then crash with "ModuleNotFoundError: No module named 'pikepdf'"

CC @dotlambda

@symphorien
Copy link
Member

I think you can wrap the executable with a PYTHONPATH that contains pikepdf

@piegamesde
Copy link
Member Author

Thanks, it works now. I grepped for PYTHONPATH and found a lot of ways of how to set it, but among the results was totem which is a Gtk application too and it uses wrapPrefixVariables = [ "PYTHONPATH" ]; which also worked for me.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
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

5 participants