Skip to content

logstash: Use jre11 instead of unsupported jre15 default#116936

Merged
SuperSandro2000 merged 3 commits intoNixOS:masterfrom
hercules-ci:logstash-jdk
Mar 30, 2021
Merged

logstash: Use jre11 instead of unsupported jre15 default#116936
SuperSandro2000 merged 3 commits intoNixOS:masterfrom
hercules-ci:logstash-jdk

Conversation

@roberth
Copy link
Member

@roberth roberth commented Mar 19, 2021 •
edited
Loading

Motivation for this change

Fix logstash and nixosTests.elk.*

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.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 19, 2021
@roberth roberth mentioned this pull request Mar 19, 2021
Closed
10 tasks
@r-rmcgibbo
Copy link

r-rmcgibbo commented Mar 19, 2021 •
edited
Loading

Result of nixpkgs-review pr 116936 at 1427a8f3 run on x86_64-linux 1

4 packages built successfully:
2 suggestions:
  • warning: build-tools-in-build-inputs

    makeWrapper is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/tools/misc/logstash/6.x.nix:32:3:

       |
32 |   buildInputs = [
   |   ^
  • warning: build-tools-in-build-inputs

    makeWrapper is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/tools/misc/logstash/7.x.nix:32:3:

       |
32 |   buildInputs = [
   |   ^

Result of nixpkgs-review pr 116936 at 1427a8f3 run on aarch64-linux 1

4 packages built successfully:
2 suggestions:
  • warning: build-tools-in-build-inputs

    makeWrapper is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/tools/misc/logstash/6.x.nix:32:3:

       |
32 |   buildInputs = [
   |   ^
  • warning: build-tools-in-build-inputs

    makeWrapper is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/tools/misc/logstash/7.x.nix:32:3:

       |
32 |   buildInputs = [
   |   ^

SuperSandro2000
SuperSandro2000 reviewed Mar 19, 2021
View reviewed changes
pkgs/tools/misc/logstash/6.x.nix Outdated
Comment on lines 6 to 13
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do you know by chance when this approach or overriding the input work jre is preferred?

Copy link
Member Author

@roberth roberth Mar 19, 2021

Choose a reason for hiding this comment

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

idk, how else would you do it?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 19, 2021

Choose a reason for hiding this comment

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

I typed a bit fast and did some typos. The alternative would be to assign jre11_headless to jre in top-level.nix for logstash.

Copy link
Member Author

@roberth roberth Mar 19, 2021

Choose a reason for hiding this comment

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

Right. I guess that would be easier to override.

@roberth roberth force-pushed the logstash-jdk branch 2 times, most recently from 9e663dd to b07d040 Compare March 22, 2021 13:22
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 22, 2021
@roberth roberth requested a review from SuperSandro2000 March 22, 2021 18:52
@roberth roberth added the 0.kind: bug Something is broken label Mar 23, 2021
@roberth roberth mentioned this pull request Mar 23, 2021
Merged
10 tasks
@roberth
Copy link
Member Author

roberth commented Mar 24, 2021

This fix is ready for review.

SuperSandro2000
SuperSandro2000 reviewed Mar 27, 2021
View reviewed changes
pkgs/tools/misc/logstash/7.x.nix
with lib;

stdenv.mkDerivation rec {
let this = stdenv.mkDerivation rec {
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 27, 2021

Choose a reason for hiding this comment

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

I am not sure why we need the let in here. Do I miss something?

Copy link
Member Author

@roberth roberth Mar 27, 2021

Choose a reason for hiding this comment

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

It's for the assertion in passthru.tests.

@roberth roberth requested review from SuperSandro2000 and removed request for wjlroe March 27, 2021 19:19
@SuperSandro2000 SuperSandro2000 merged commit 7d0dad7 into NixOS:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@offlinehacker offlinehacker Awaiting requested review from offlinehacker

@basvandijk basvandijk Awaiting requested review from basvandijk

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees

No one assigned

Labels

0.kind: bug Something is broken 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@roberth @r-rmcgibbo @SuperSandro2000