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

build and develop using Nix package manager #1307

Merged
merged 9 commits into from Nov 21, 2023
Merged

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Nov 17, 2023

This PR provides a Nix build for v6+ based off nixpkgs' ulauncher build

summing up it allows you to:

  • use v6 branch externally in your own Nix configurations (be it NixOS or just a Nix package manager),
  • develop locally with Nix-built Python env (not sure how much is it real virtualenv),
  • continously lint and test whenever rebuilding the code (during packaging step),

nazarewk added a commit to nazarewk-iac/nix-configs that referenced this pull request Nov 17, 2023
see Ulauncher/Ulauncher#1307

Signed-off-by: Krzysztof Nazarewski <gpg@kdn.im>
@nazarewk
Copy link
Contributor Author

nazarewk commented Nov 17, 2023

I'm quite a lot of dependencies can be removed compared to v5, but I'm not sure which, could you help me with it?

also seems like I'm not getting an app indicator icon when doing nix run '.#ulauncher6', but it works fine in nazarewk-iac/nix-configs@cc56c99 so I'm not sure if I'll investigate at all

@nazarewk
Copy link
Contributor Author

nazarewk commented Nov 17, 2023

regarding #1306 (comment) :

Would anyone be opposed to developing a base Nix package/development environment in this repo instead? It would involve a few Nix files and nix-specific patches.

Since the current maintainers don't know how to maintain that code I think it's better if it's not in the repo, but maybe for the future we can set things up to make releases that update the Nix package similar to how we do with the Arch AUR: v6/.github/workflows/publish-release.yml#L36. If we need to host files I think in that it would be better with a separate repo though. I want to move all the debian packaging stuff to a separate repo as well.

Actually it would be a good idea to create a CI pipeline to update nixpkgs for v6 after it is released (this would most likely just be a copy of the local-repo config with slight adjustments).

It doesn't stop us from providing OOTB build inside the repo as Nix itself is fully source-based:

  1. /default.nix is what you would consider a "package" equivalent in a Nix world, it is used by non-flake configurations,
  2. /flake.nix is de-facto modern standard to pin and reuse software from arbitrary sources, it allows me to:
    • use the package like this
    • easily update to latest version with nix flake update or nix flake lock --update-input ulauncher

We'll see how I like the Ulauncher itself, but I might be able to maintain this in-repo Nix piece.

@friday
Copy link
Member

friday commented Nov 17, 2023

Thanks again for your work. I think I will try to move the deb build to a separate repo in the weekend, and we can push this PR there as well, but in the meanwhile feel free to keep this PR up. If you want an initial working draft with everything in one place I think this makes sense as a first step (part of it should go in here after all).

A lot of the deps has indeed changed, so you can remove these:

And these optional deps have been added:

@nazarewk nazarewk force-pushed the nix-build branch 5 times, most recently from 9e621d6 to 5efc6b3 Compare November 17, 2023 12:06
@friday
Copy link
Member

friday commented Nov 18, 2023

I see you made a lot of changes to the program logic as well. The preferred way to get these merged is to make individual PRs for each isolated change, justifying why they are needed (if they are general improvements, which some of them seem to be).

Alternatively if you comment on that here I can cherry-pick the changes manually, but then you don't get the official credit.

For Nix-specific changes that change the behavior in ways they weren't built for originally, we should probably discuss how to get them merged. For example by providing options to the setup script so it can be enabled on Nix, but not other distros.

Also, some of the code you have changed here is subject to future refactoring, I have considered replacing the build system because the way we do it now is deprecated and throws lots of warnings. The official pythonic way seems to be to move the spec from setup.cfg to pyproject.toml and I have also seen projects without any pyproject.toml or setup.cfg/py, using meson instead. Let me know if you have recommendations that work well with Nix (although I am moving forward with pyproject.toml for an upcoming PR).

@nazarewk
Copy link
Contributor Author

nazarewk commented Nov 18, 2023

Yeah, I am making some general improvements that I might create separate PRs for, wanted to experiment first and see if those work at all and how much effort is required. I do not have any tests (i just built everything and run to see it work) or discussed those yet, just made sure to just augment without breaking original behaviours.

Nix build doesn't strictly require any changes done, it could work around most stuff encountered in the wild. This is the first time I package anything Python the raw nixpkgs way (main repo), only used poetry2nix so far. I contain all Nix-specific patches/replacements that don't make sense upstream within build configs.

About recommendations:

  • i've seen all kinds of python builds working in nixpkgs: setup.py, pyproject, meson and poetry. So anything would work. I'm not really up to date with Python packaging except poetry: last time i made packaging decisions or research was more than 6 years ago and poetry2nix is the easiest way to pin exact dependency versions in Nix (as opposed to using a single nixpkgs-wide versions) so I stick with it.
  • Nix symlinks everything all the time (first change)
  • Nix prefers to load plugins/extensions (and writing those was my primary motivation to try out ulauncher) from arbitrary standard paths or at least from within the parent package (symlinking to /share inside the package), this motivated my XDG_CONFIG_DIRS handling. Otherwise I would have to rely on home-manager (user/home dotfiles and configs manager) to symlink to HOME

I can expand on the topics or split out changes into separate PRs to merge quickly as-is on Monday.

@friday
Copy link
Member

friday commented Nov 19, 2023

I pushed and merged three PRs today which relate to the build system and/or our communication (but only one small conflict with what you've been working on).

I'm not planning to do more work that can conflict with your work now, unless you ask me to.

@nazarewk
Copy link
Contributor Author

@friday Would you be up form merging this PR without XDG_DATA_DIRS change? Would make it significantly easier for me to work with the repo in another PRs.

Today I have:

  1. made the full tests suite run during Nix rebuild (caught some errors),
  2. finally succeeded in making a Python binary (actually some kind of env which I don't think is virtualenv at all) that I can point the IDE (Jetbrains here) to as a development interpreter,
  3. added a few changes to the code enabling above 2

@nazarewk nazarewk force-pushed the nix-build branch 2 times, most recently from bb73413 to f239d49 Compare November 21, 2023 15:26
@nazarewk nazarewk marked this pull request as ready for review November 21, 2023 15:29
- primarily to point IDEs (at least Jetbrains family) to it
- this is some kind of env, but most likely not a `virtualenv`
- to save time it embeds a full build of the package into env, IDEs should handle it easily by prepending PWD to PYTHONPATH,
@nazarewk
Copy link
Contributor Author

I have dropped #1319 code from this PR and made it ready to be merged.

There are 3 remaining changes, but I don't think they warrant creating separate PRs:

  • I had to edit tests/modes/shortcuts/test_RunScript.py so replacing /bin/bash would not induce black errors
  • ./ul test-pytest never failed due to set +x, command -v will not include results without executable bit set, so the check was redundant,
  • used --auto-servernum instead of DISPLAY=:99 in xvfb-run invocation, see https://man.archlinux.org/man/xvfb-run.1.en#a

@nazarewk nazarewk changed the title Nix build configuration init: build and develop using Nix package manager Nov 21, 2023
@nazarewk nazarewk changed the title init: build and develop using Nix package manager build and develop using Nix package manager Nov 21, 2023
Copy link
Member

@friday friday left a comment

Choose a reason for hiding this comment

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

Looks great 👍 Thank you for committing to all this work 🍻

Also nice with the test script improvement (--auto-servernum) I didn't know about that one.

As mention I was planning to move the packaging to a separate repo in the future as this was on a long checklist requested by a Debian packager, and I think it also would be nice to separate the app release workflow from the build, so if the build crashes you can fix it without having to make a new release or faking one.

@friday friday merged commit 361244b into Ulauncher:v6 Nov 21, 2023
1 check passed
@nazarewk nazarewk deleted the nix-build branch November 22, 2023 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants