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

jq: build from Git tag (instead of dist tarball) #71375

Closed
wants to merge 1 commit into from

Conversation

bb010g
Copy link
Member

@bb010g bb010g commented Oct 19, 2019

This involves vendoring the Bundler specification for the docs's Rake invocation for now, but next release will mean a switch to Python (patching ~the same files). (I've already done this on Python for docs building, and it's kind of easier. Less vendoring for sure.)

We also now check with Valgrind by default, as per upstream. This takes longer. (We can turn this off by default if necessary to restore old installCheck times.)

Motivation for this change

I wanted to try out an unstable version of jq, and needed to make most of these changes anyways.

As per #31236, building from source (here, Git) should be encouraged over building from release or distribution tarballs. This makes it easier to patch the derivation to use alternative VCS branches, among other benefits.

Valgrind is on by default in the Makefile, but we haven't ran the tests with it because they automatically turned it off when dependency discovery failed.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)

  • 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 nix-review --run "nix-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)

    out bin lib man doc dev
    before 96 29_528_224 29_492_112 32_064 14_568 29_541_768
    after 96 (+0) 29_528_224 (±0) 29_492_112 (±0) 32_784 (+720) 14_568 (±0) 29_541_768 (±0)

    (Building with { enableValgrindChecks = false; } does not change the closure size.)

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

Notify maintainers

cc @7c6f434c @globin


This change is Reviewable

@bb010g
Copy link
Member Author

bb010g commented Oct 19, 2019

This file contains what the Python transition will likely look like, the meat being:

https://github.com/bb010g/nur-packages/blob/d212b8f1bf018754b515efd979dca9e5df6e8e16/pkgs/development/tools/jq/default.nix#L19-L25

let
  # from ./docs/Pipfile{,.lock}
  docsPythonPackages = ps: [
    ps.jinja2
    ps.lxml
    ps.markdown
    ps.pyyaml_3 or ps.pyyaml
  ];
in

(I'm not sure how to do this like with Ruby and respect the Pipenv.lock.)

@Mic92
Copy link
Member

Mic92 commented Oct 19, 2019

This file contains what the Python transition will likely look like, the meat being:

bb010g/nur-packages:pkgs/development/tools/jq/default.nix@d212b8f#L19-L25

let
  # from ./docs/Pipfile{,.lock}
  docsPythonPackages = ps: [
    ps.jinja2
    ps.lxml
    ps.markdown
    ps.pyyaml_3 or ps.pyyaml
  ];
in

(I'm not sure how to do this like with Ruby and respect the Pipenv.lock.)

That looks simple enough. Let's not mess with pipenv. I remember having headaches from the last time I used it.

@risicle
Copy link
Contributor

risicle commented Oct 20, 2019

jq fails to build for me on macos 10.13:

FAIL: tests/optionaltest
FAIL: tests/mantest
FAIL: tests/jqtest
FAIL: tests/onigtest
FAIL: tests/shtest
PASS: tests/utf8test
FAIL: tests/base64test
============================================================================
Testsuite summary for jq 1.6
============================================================================
# TOTAL: 7
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  6
# XPASS: 0
# ERROR: 0
============================================================================

@bb010g
Copy link
Member Author

bb010g commented Oct 20, 2019

@risicle Could you build with {-K | --keep-failed} and paste the relevant test log output? (If it's long, GitHub lets you surround it in a <details> block (with empty one-line buffers between the HTML opening & closing tags and the contained Markdown fenced code blocks for the parser).) I'm guessing Valgrind isn't happy, and we never ran into this before?

@veprbl
Copy link
Member

veprbl commented Dec 8, 2019

On darwin I get all seven tests fail with

valgrind: mmap-FIXED(0x7fff5f400000, 8388608) failed in UME (load_unixthread1) with error 22 (Invalid argument).

@stale
Copy link

stale bot commented Jun 5, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 5, 2020
@SuperSandro2000
Copy link
Member

@bb010g please resolve the merge conflict.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 18, 2021
@SuperSandro2000 SuperSandro2000 added 2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Jan 18, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 18, 2021
@bb010g
Copy link
Member Author

bb010g commented Jan 18, 2021

Rebased. Valgrind checks are no longer enabled by default, which should fix macOS builds.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 71375 run on x86_64-linux 1

4 packages marked as broken and skipped:
  • linuxPackages_4_4.sysdig
  • linuxPackages_4_9.sysdig
  • linuxPackages_hardkernel_4_14.sysdig
  • linuxPackages_hardkernel_latest.sysdig
2 packages blacklisted:
  • tests.nixos-functions.nixos-test
  • tests.nixos-functions.nixosTest-test
4 packages failed to build:
  • linuxPackages-libre.sysdig
  • oq
  • plex-media-player
  • vscode-extensions.vadimcn.vscode-lldb
104 packages built:
  • anki
  • ankisyncd
  • bash-my-aws
  • cargo-download
  • carnix
  • celluloid
  • chrome-gnome-shell
  • code-server
  • common-updater-scripts
  • corectrl
  • curaLulzbot
  • curseradio
  • disnix
  • disnixos
  • dune-release
  • dydisnix
  • element-desktop
  • glslang
  • google-cloud-sdk (google-cloud-sdk-gce)
  • hydra-unstable
  • i3-layout-manager
  • imgur-screenshot
  • jellyfin-mpv-shim
  • jftui
  • jq
  • kodi-cli
  • libplacebo
  • linuxPackages.sysdig (linuxPackages_5_4.sysdig)
  • linuxPackages_4_14.sysdig
  • linuxPackages_4_19.sysdig
  • linuxPackages_5_10.sysdig (linuxPackages_latest.sysdig)
  • linuxPackages_5_9.sysdig
  • linuxPackages_hardened.sysdig
  • linuxPackages_latest-libre.sysdig
  • linuxPackages_latest_hardened.sysdig
  • linuxPackages_latest_xen_dom0.sysdig
  • linuxPackages_latest_xen_dom0_hardened.sysdig
  • linuxPackages_lqx.sysdig
  • linuxPackages_testing_bcachefs.sysdig
  • linuxPackages_xen_dom0.sysdig
  • linuxPackages_xen_dom0_hardened.sysdig
  • linuxPackages_zen.sysdig
  • lorri
  • lutris
  • lutris-free
  • lutris-unwrapped
  • memtest86-efi
  • minitube
  • mnemosyne
  • monado
  • mpc-qt
  • mpv
  • mpv-unwrapped
  • mpvScripts.mpris
  • nix-prefetch
  • nix-prefetch-docker
  • nix-update
  • nixFlakes (nixUnstable)
  • nixos-generators
  • nixos-rebuild
  • nixpkgs-review
  • ocamlformat (ocamlformat_0_15_0)
  • ocamlformat_0_11_0
  • ocamlformat_0_12
  • ocamlformat_0_13_0
  • ocamlformat_0_14_0
  • ocamlformat_0_14_1
  • ocamlformat_0_14_2
  • ocamlformat_0_14_3
  • pb_cli
  • playonlinux
  • plex-mpv-shim
  • python37Packages.jq
  • python37Packages.mpv
  • python37Packages.yq
  • python38Packages.jq
  • python38Packages.mpv
  • yq (python38Packages.yq)
  • python39Packages.jq
  • python39Packages.mpv
  • python39Packages.yq
  • qimgv
  • qmplay2
  • r2mod_cli
  • rmount
  • rofi-systemd
  • slack-cli
  • somafm-cli
  • sublime-music
  • sway-contrib.grimshot
  • swiftshader
  • sysdig
  • systemfd
  • todoman
  • toml2nix
  • tuijam
  • twa
  • vscode-extensions.matklad.rust-analyzer
  • vscode-extensions.ms-vscode.cpptools
  • vulkan-extension-layer
  • vulkan-tools
  • vulkan-tools-lunarg
  • vulkan-validation-layers
  • xrgears

}:

assert enableValgrindChecks -> valgrind != null;
assert enableValgrindChecks -> which != null;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just have less/valgrind/which in the arguments without ? null and drop this assertion? They won't be used anyway except conditionally (and evaluation is lazy anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this pattern meant for callPackage on the file's expression from scopes that don't necessarily have optional package arguments present, though? Destructuring attrset arguments eagerly check for their named parameters. Admittedly, it's a niche case, but without an overall style decision in Nixpkgs I'd prefer to keep with the usual style (that has minor technical merits).

The assertions are there for clearer error messages, and are a pattern (to some extent) throughout Nixpkgs too AFAIK.

Thanks for reminding me to take a look at less, not sure actually why that's there at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Think I included less for $src/tests/mantest, but it runs fine without it. Removed.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep with the usual style

Usually style is to add an enable flag and use lib.optionals.

As per NixOS#31236, building from source (here, Git) should be
encouraged over building from release or distribution tarballs. This
makes it easier to patch the derivation to use alternative VCS branches,
among other benefits.

This involves vendoring the Bundler specification for the docs's Rake
invocation for now, but next release will mean a switch to Python
(patching approximately the same files).
@bb010g
Copy link
Member Author

bb010g commented Jan 19, 2021

Result of nixpkgs-review pr 71375 run on x86_64-linux 1
4 packages failed to build:

* linuxPackages-libre.sysdig

* oq

* plex-media-player

* vscode-extensions.vadimcn.vscode-lldb

It doesn't look like any of these failures are due to jq.

Comment on lines +89 to +95
meta = {
description = ''A lightweight and flexible command-line JSON processor'';
downloadPage = "https://stedolan.github.io/jq/download/";
homepage = "https://stedolan.github.io/jq/";
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ raskin globin bb010g ];
platforms = with lib.platforms; linux ++ darwin;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
description = ''A lightweight and flexible command-line JSON processor'';
downloadPage = "https://stedolan.github.io/jq/download/";
homepage = "https://stedolan.github.io/jq/";
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ raskin globin bb010g ];
platforms = with lib.platforms; linux ++ darwin;
meta = with lib; {
description = ''A lightweight and flexible command-line JSON processor'';
downloadPage = "https://stedolan.github.io/jq/download/";
homepage = "https://stedolan.github.io/jq/";
license = licenses.mit;
maintainers = with maintainers; [ raskin globin bb010g ];
platforms = platforms.unix;

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that last line would work.

Comment on lines +11 to +12
assert enableValgrindChecks -> valgrind != null;
assert enableValgrindChecks -> which != null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert enableValgrindChecks -> valgrind != null;
assert enableValgrindChecks -> which != null;

This is not required and breaks overriding in certain cases.

Comment on lines +4 to +8
, bundlerApp ? null
# features
# (enabling Valgrind checks for testing severly increases build time)
, enableValgrindChecks ? false
, valgrind ? null, which ? null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
, bundlerApp ? null
# features
# (enabling Valgrind checks for testing severly increases build time)
, enableValgrindChecks ? false
, valgrind ? null, which ? null
, bundlerApp
# features
# (enabling Valgrind checks for testing severly increases build time)
, enableValgrindChecks ? false
, valgrind, which

@stale
Copy link

stale bot commented Aug 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 13, 2021
@sarcasticadmin
Copy link
Member

Leveraging the git tag seems to have been fixed in #117895

I think this can be closed

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 18, 2021
@Atemu Atemu closed this Aug 18, 2021
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

8 participants