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

dbvisualizer: init at 24.2.2 #344225

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

boldikoller
Copy link

@boldikoller boldikoller commented Sep 24, 2024

Description of changes

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Sep 24, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Sep 24, 2024
@boldikoller boldikoller force-pushed the init-dbvisualizer branch 2 times, most recently from 73cc7fe to 4e2ff38 Compare September 24, 2024 14:45
@momeemt
Copy link
Member

momeemt commented Sep 24, 2024

Result of nixpkgs-review pr 344225 run on aarch64-darwin 1

1 package built:
  • dbvisualizer

Copy link
Member

@momeemt momeemt left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I left the comment about mainProgram, besides this, lgtm.

pkgs/by-name/db/dbvisualizer/package.nix Show resolved Hide resolved
Copy link
Member

@momeemt momeemt left a comment

Choose a reason for hiding this comment

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

You should also split the commit into maintainers: add boldikoller and dbvisualizer: init at 24.2.2. ref

@boldikoller boldikoller force-pushed the init-dbvisualizer branch 2 times, most recently from e9eac3f to 44ae773 Compare September 24, 2024 15:01
@boldikoller
Copy link
Author

You should also split the commit into maintainers: add boldikoller and dbvisualizer: init at 24.2.2. ref

Should be fixed, thanks again!

Copy link
Member

@momeemt momeemt left a comment

Choose a reason for hiding this comment

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

Thank you for your work! LGTM.

@momeemt momeemt added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 24, 2024
pkgs/by-name/db/dbvisualizer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/db/dbvisualizer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/db/dbvisualizer/package.nix Outdated Show resolved Hide resolved
@boldikoller
Copy link
Author

Would it be better to set env.strictDeps as opposed to just strictDeps?

@Bot-wxt1221
Copy link
Member

@boldikoller I wonder why we should define strictDeps.

@boldikoller
Copy link
Author

boldikoller commented Oct 1, 2024

@boldikoller I wonder why we should define strictDeps.

I am sorry, I must be misunderstanding something. I previously understood strictDeps turned on as a setting to tell Nix to separate buildInputs from nativeBuildInputs. So that nativeBuildInputs are not also runtime dependencies (although the nativeBuildInputs not used at runtime would end up being stripped out either way). Is that incorrect?

Also, is strictDeps only read by the builder script from the environment variables? And not at all used on the Nix language side? If that is the case, then I can understand why to instead use env.strictDeps. However, I could find no documentation about whether that is the case or not. Excuse me if that is on me.

Edit: If this is the case, then I must be doing something wrong, because it does not even build with env.strictDeps = true with the error of "The env attribute set cannot contain any attributes passed to derivation.".

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 7, 2024
@boldikoller
Copy link
Author

Is there anything that still needs to be changed?

@Bot-wxt1221
Copy link
Member

@boldikoller always fix ofborg first.


nativeBuildInputs = [ makeWrapper ];

strictDeps = true;
Copy link
Member

Choose a reason for hiding this comment

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

Please tell us why this.

Copy link
Author

Choose a reason for hiding this comment

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

I enabled strictDeps because I was pretty sure it is considered a best practise to enable it? Is it not? Should I disable it? Let me try if that fixes ofborg

@boldikoller
Copy link
Author

@boldikoller always fix ofborg first.

@Bot-wxt1221 I do not know how to fix this. The error ofborg gives is error: access to URI 'https://www.dbvis.com/product_download/dbvis-24.2.2/media/dbvis_linux_24_2_2.tar.gz' is forbidden in restricted mode.

Isn't it ran in restricted mode because the license is unfree? Or am I wrong about this?

I do not know how I should go about fixing this

@Bot-wxt1221
Copy link
Member

@boldikoller Please run nix build /path/to/nixpkgs#package-name to check whether it can pass build.

@boldikoller
Copy link
Author

@boldikoller Please run nix build /path/to/nixpkgs#package-name to check whether it can pass build.

@Bot-wxt1221 It builds just fine locally. I've tested before with nix-build /path/to/nixpkgs -A dbvisualizer, and I get the same result with nix build /path/to/nixpkgs#dbvisualizer.

Are you sure the reason ofborg failing is not because the license is set to unfree?

@Bot-wxt1221
Copy link
Member

@boldikoller Not because of unfree.

@boldikoller
Copy link
Author

@boldikoller always fix ofborg first.

@Bot-wxt1221 ofborg has been fixed it.

@Bot-wxt1221 Bot-wxt1221 self-requested a review November 15, 2024 15:12
@boldikoller boldikoller force-pushed the init-dbvisualizer branch 4 times, most recently from b5ae92d to 8bf2817 Compare November 18, 2024 13:46
makeDesktopItem,
makeWrapper,
stdenv,
temurin-jre-bin-17,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have to be Temurin? (i.e. does openjdk17 work?)

Copy link
Author

Choose a reason for hiding this comment

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

It does not need to be temurin. My reasoning was that temurin-jre-bin-17 is 136MB as opposed to openjdk17 which is 470MB. Makes sense, since it is just the runtime as opposed to an sdk.

Probably there is a way to take only the jre from openjdk? When looking through nixpkgs I only found jre17_minimal as a potential candidate, but I was not sure what minimal implied. And sure enough, it does not run with jre17_minimal. Thus I just went for temurin.

Copy link
Author

Choose a reason for hiding this comment

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

Otherwise, seems to run just fine with openjdk17.

Copy link
Contributor

@pluiedev pluiedev Nov 22, 2024

Choose a reason for hiding this comment

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

We removed the JRE-only builds from Nixpkgs a while back as a part of an overall refactor. OpenJDK is preferred since it's something that we built ourselves, rather than Temurin which is a prebuilt, binary-only distribution, and that it's the generic "baseline" for Java packages in Nixpkgs — if there's no special requirements then Nixpkgs packages should always use OpenJDK

Copy link
Author

Choose a reason for hiding this comment

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

Alright then, in the name of uniformity I concede.

It would be nice if there was a way to only include the JRE, however. Maybe as an argument to the openjdk package in the future? As an override? Ultimately, it's not a big deal, but in this case it basically doubles the download size, which is suboptimal.

Copy link
Contributor

Choose a reason for hiding this comment

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

After doing some more digging, I think you might be able to make jre_minimal work by overriding it with more modules, like jre17_minimal.override { modules = [ "java.base" "java.logging" ]; }. Yes, this is undocumented, yes, this sucks, I'll probably make a separate issue about it

pkgs/by-name/db/dbvisualizer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/db/dbvisualizer/package.nix Show resolved Hide resolved
Comment on lines 60 to 61
ln -s $out/dbvis $out/bin/
wrapProgram $out/bin/dbvis --set INSTALL4J_JAVA_HOME ${temurin-jre-bin-17}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be symlinked, or simply renamed?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question. In my opinion either can make sense. My reasoning behind symlinking it is that I did not want to modify the layout of the tarball, only add onto it. Maybe that is just silly and it should be mvd instead.

@boldikoller boldikoller force-pushed the init-dbvisualizer branch 2 times, most recently from 5e0f410 to 76994db Compare November 22, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants