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

gradle2nix: init #77422

Draft
wants to merge 5 commits into
base: master
from
Draft

gradle2nix: init #77422

wants to merge 5 commits into from

Conversation

@petabyteboy
Copy link
Member

@petabyteboy petabyteboy commented Jan 10, 2020

Motivation for this change

Based on #77417
@tadfisher has done a wonderful job on gradle2nix and this is an attempt to integrate it into nixpkgs.
I have chosen a strategy similar to the one used for yarn2nix: Only the required .nix files are copied from the upstream repository to nixpkgs, the nix code generator itself remains in a seperate repository and is packaged as a normal application.

Todo:

  • reintroduce mxisd package tests as checkPhase
  • debloat gradle2nix output
  • change gradle2nix version to something other than "unspecified"
  • ?
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@petabyteboy petabyteboy changed the title Feature/gradle2nix gradle2nix: init Jan 10, 2020
@ofborg ofborg bot requested review from thoughtpolice, fgaz and mguentner Jan 10, 2020
@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Jan 10, 2020

It seems like we are still having some IFD problems or something similar.

@volth
Copy link
Contributor

@volth volth commented Jan 10, 2020

Recursive Nix (NixOS/nix#3205) might be a solution to avoid including big .json files into nixpkgs.
I am not sure, just an idea to investigate

@tadfisher
Copy link
Contributor

@tadfisher tadfisher commented Jan 14, 2020

Looks like everything built fine except ma1sd:

A problem occurred configuring root project 'source'.
> Could not resolve all artifacts for configuration ':classpath'.
   > Could not resolve com.github.ben-manes:gradle-versions-plugin:0.21.0.
     Required by:
         project :
      > No cached version of com.github.ben-manes:gradle-versions-plugin:0.21.0 available for offline mode.

This is strange because it depends on version 0.27.0 and gradle2nix resolved that correctly. I'll try and reproduce here.

Copy link
Member

@fgaz fgaz left a comment

Reviewed the changes to shattered-pixel-dungeon. I still have to actually build and test it.
edit: oops, I didn't notice the WIP status

pkgs/games/shattered-pixel-dungeon/default.nix Outdated Show resolved Hide resolved
pkgs/games/shattered-pixel-dungeon/default.nix Outdated Show resolved Hide resolved
pkgs/games/shattered-pixel-dungeon/default.nix Outdated Show resolved Hide resolved
pkgs/games/shattered-pixel-dungeon/default.nix Outdated Show resolved Hide resolved
@tadfisher
Copy link
Contributor

@tadfisher tadfisher commented Jan 26, 2020

@petabyteboy Looks like ma1sd was checked out at the 2.2.1 branch instead of 2.1.1.

Also, I have a concern with the approach here. Since the same gradle-env.nix is used for each package using buildGradle, I won't be able to change the JSON schema for gradle-env.json. I've already done so for gradle2nix 1.0.0-rc1, which would require an update to every package using buildGradle, both in nixpkgs and for out-of-tree callers importing buildGradle from nixpkgs.

So there are a few options here:

  1. Do nothing, and maybe leave a comment in the gradle2nix derivation that all buildGradle callers need to have gradle2nix re-run to generate a new gradle-env JSON file.
  2. Make an updateScript function, similar to gnome3.updateScript, which calls gradle2nix when updating buildGradle packages, making it a bit more convenient to update gradle-env.json files all-at-once when updating gradle2nix itself.
  3. Bundle gradle-env.nix with each package, instead of using the yarn2nix-style approach.
  4. Modify gradle2nix to be a bit more convenient to bundle in nixpkgs. For example, flattening gradle-env.json into gradle-env.nix and making the latter a single file we can bundle in nixpkgs.

Thoughts?

@mguentner
Copy link
Contributor

@mguentner mguentner commented Jan 26, 2020

I packaged mxisd / ma1sd a while ago but stopped using both packages due to architectural changes in the matrix protocol (not relevant to this PR). This however is the reason why I am not paying much attention to the packages atm. I would be fine with removing at least mxisd soon and hope that somebody cares for ma1sd enough. Maybe some other user of ma1sd wants to take over!

@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Jan 28, 2020

@petabyteboy Looks like ma1sd was checked out at the 2.2.1 branch instead of 2.1.1.

Also, I have a concern with the approach here. Since the same gradle-env.nix is used for each package using buildGradle, I won't be able to change the JSON schema for gradle-env.json. I've already done so for gradle2nix 1.0.0-rc1, which would require an update to every package using buildGradle, both in nixpkgs and for out-of-tree callers importing buildGradle from nixpkgs.

Thanks for noticing, I did not really think about this.
I think we should definitely get the changes done to the format in gradle2nix 1.0.0-rc1 into this PR. Of course it would be nice to see a more stable format, but if it does require another change, ...

So there are a few options here:

  1. Do nothing, and maybe leave a comment in the gradle2nix derivation that all buildGradle callers need to have gradle2nix re-run to generate a new gradle-env JSON file.

... I think option 1. is the best. since it is okay to require all gradle2nix applications to be updated when gradle2nix is has changes to the gradle-env.json format. Additionally I will add an updater script to each individual package to make this easier.

  1. Make an updateScript function, similar to gnome3.updateScript, which calls gradle2nix when updating buildGradle packages, making it a bit more convenient to update gradle-env.json files all-at-once when updating gradle2nix itself.

I already wanted to implement an updater script for each package even without the possibility of changes to gradle-env.json, but now it is even more useful.

  1. Bundle gradle-env.nix with each package, instead of using the yarn2nix-style approach.

I am strongly opposed, since this would lead to code duplication, and in the end it would not be less work to do a proper upgrade of gradle2nix.

  1. Modify gradle2nix to be a bit more convenient to bundle in nixpkgs. For example, flattening gradle-env.json into gradle-env.nix and making the latter a single file we can bundle in nixpkgs.

I am not sure what exactly you mean by this. I think the gradle2nix application itself is already easy to package. For other packages using buildGradle, I would see it as a big disadvantage if we have to duplicate the whole buildGradle expression for each package, so I am very happy with the current approach.

Thoughts?

@petabyteboy petabyteboy force-pushed the petabyteboy:feature/gradle2nix branch from f69956a to b15cfc8 Jan 28, 2020
@petabyteboy petabyteboy requested a review from fgaz Jan 28, 2020
@tadfisher
Copy link
Contributor

@tadfisher tadfisher commented Jan 28, 2020

... I think option 1. is the best. since it is okay to require all gradle2nix applications to be updated when gradle2nix is has changes to the gradle-env.json format. Additionally I will add an updater script to each individual package to make this easier.

Sounds good!

There's also the possibility to include separate versions of gradle2nix if we need to make incompatible changes in the future, although I don't forsee this happening. Not a big deal for this PR, of course.

I wrote a skeleton updateScript wrapper expression, if helps, though I haven't tested it: https://gist.github.com/tadfisher/1007f68d321d6789dfd3b2301119f685

@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Jan 28, 2020

Oh, that's neat. I already wrote updater-scripts for ma1sd and mxisd and fixed the NixOS test for them.
But it's not really duplicated work since your update script wouldn't work for them as it is, because they require the .git directory to be present when running gradle2nix, which is not present in the derivation source.

@petabyteboy petabyteboy force-pushed the petabyteboy:feature/gradle2nix branch from b15cfc8 to 29bc813 Jan 28, 2020
@petabyteboy petabyteboy force-pushed the petabyteboy:feature/gradle2nix branch from 29bc813 to 0e3502a Jan 28, 2020
@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Jan 28, 2020

I have noticed there is one big caveat to using update scripts:
When the gradle-env.json format changes, the update script must be called with the new gradle2nix env generator, but it must not try to build it with the new gradle-env.nix or it will fail.
The script I wrote for mxisd and ma1sd work fine, because they don't try to evaluate the derivation.
I will think of something to avoid this, without giving up too much of the advantages that using nix-shell provides.

@tadfisher
Copy link
Contributor

@tadfisher tadfisher commented Feb 20, 2020

FYI, I pushed 1.0.0-rc2 which fixes a breaking behavior for Kotlin-based projects.

@samrose samrose mentioned this pull request Apr 9, 2020
0 of 10 tasks complete
@stale
Copy link

@stale stale bot commented Aug 19, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale label Aug 19, 2020
@Atemu
Copy link
Member

@Atemu Atemu commented Aug 19, 2020

Still important te me.

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.