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

nodePackages.carto: init at 1.2.0 #70226

Closed
wants to merge 2 commits into from
Closed

Conversation

jglukasik
Copy link
Contributor

Motivation for this change

This package builds a standalone binary, carto that is used to generate stylesheets for OpenStreetMaps

Things done

Followed the steps for adding new node packages at https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/node.section.md

  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

regression in sage package

builder for '/nix/store/yf4bdpddbpl9waxn3lq5aq12zrhn388m-sage-tests-8.9.drv' failed with exit code 1; last 10 log lines:
      [750 tests, 339.72 s]
  ----------------------------------------------------------------------
  sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/plot/plot3d/base.pyx  # 1 doctest failed
  sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/backend_ipython.py  # 1 doctest failed
  sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/display_manager.py  # 1 doctest failed
  sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/backend_sagenb.py  # 1 doctest failed
  ----------------------------------------------------------------------
  Total time for all tests: 1083.6 seconds
      cpu time: 11038.8 seconds
      cumulative wall time: 23116.3 seconds
cannot build derivation '/nix/store/rkc5r6h0dxccwpzw6yckfrgx39s4mzmm-sage-8.9.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/v9rra2ry86iwwp4jlrf3zchvnsg00yry-sage-8.9.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/vw4aqi58qbfkxqf5lwfggrwj2l94qrq2-env.drv': 3 dependencies couldn't be built
[87 built (2 failed)]
error: build of '/nix/store/vw4aqi58qbfkxqf5lwfggrwj2l94qrq2-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/70226
2 package are marked as broken and were skipped:
casperjs meguca

3 package failed to build:
azure-cli sage sageWithDoc

13 package were build:
antora create-cycle-app gtop iosevka joplin lumo parity-ui rambox-pro shout slack slack-dark thelounge wasm-text-gen

@jglukasik
Copy link
Contributor Author

Thanks for catching that @jonringer !

Running `nix-build -A sage` locally, I saw some errors related to three.js
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/backend_sagenb.py
**********************************************************************
File "/nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/backend_sagenb.py", line 468, in sage.repl.r
ich_output.backend_sagenb.BackendSageNB.threejs_offline_scripts
Failed example:
    backend.threejs_offline_scripts()
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 6
81, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 1
131, in compile_and_execute
        exec(compiled, globs)
      File "", line 1, in 
        backend.threejs_offline_scripts()
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/repl/rich_output/backend_s
agenb.py", line 472, in threejs_offline_scripts
        return get_display_manager().threejs_scripts(online=True)
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/repl/rich_output/display_m
anager.py", line 751, in threejs_scripts
        version = re.search(r'REVISION="(\d+)"', text).group(1)
    AttributeError: 'NoneType' object has no attribute 'group'
**********************************************************************
1 item had failures:
   1 of   4 in sage.repl.rich_output.backend_sagenb.BackendSageNB.threejs_offline_scripts
    [78 tests, 1 failure, 1.58 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/output_catalog.py
    [0 tests, 0.00 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/output_graphics.py
    [38 tests, 0.05 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/output_graphics3d.py
    [46 tests, 0.07 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/output_video.py
    [25 tests, 0.06 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/preferences.py
    [68 tests, 0.11 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/backend_doctest.py
    [58 tests, 4.50 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/user_globals.py
    [36 tests, 0.07 s]
sage -t --long /nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/display_manager.py
**********************************************************************
File "/nix/store/s0v48jrmg8s4rvphszxl92ddvwlq6jqk-sage-src-8.9/src/sage/repl/rich_output/display_manager.py", line 739, in sage.repl.
rich_output.display_manager.DisplayManager.threejs_scripts
Failed example:
    get_display_manager().threejs_scripts(online=True)
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 6
81, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 1
131, in compile_and_execute
        exec(compiled, globs)
      File "", line 1, in 
        get_display_manager().threejs_scripts(online=True)
      File "/nix/store/vlgb8wdll5ypm74kg7fiq6imc4wh3s72-python-2.7.16-env/lib/python2.7/site-packages/sage/repl/rich_output/display_m
anager.py", line 751, in threejs_scripts
        version = re.search(r'REVISION="(\d+)"', text).group(1)
    AttributeError: 'NoneType' object has no attribute 'group'
**********************************************************************
1 item had failures:
   1 of   4 in sage.repl.rich_output.display_manager.DisplayManager.threejs_scripts

In the sage tests, the three.js version is obtained by looking for REVISION="(\d+)" in the compiled three.min.js file. But, three.js version 109 has a regression where this number is no longer set. A PR has been accepted to fix this for the next version ( mrdoob/three.js#17342 ), but the latest version is broken, and that is what generate.sh upgraded to.

After pinning three to version 108 in node-packages-v10.json, and running generate.sh again, i confirm that nix-build -A sage works and the tests pass.

I think its not ideal that adding a new node package automatically upgrades all the others. It makes it very hard to spot regressions like this that are unrelated to the package being added. Also, since the "source of truth" here is a json file, i can't add a comment explaining why i'm pinning three.

My plan now is to add a new commit to explicitly pins three.js, with a commit message explaining why. This pinning doesn't seem to be widely used though.

cc @svanderburg for your node2nix work, does this seem OK to you, or do you recommend any other strategy?

@jglukasik
Copy link
Contributor Author

jglukasik commented Oct 11, 2019

Whoops, spoke too soon 🙂 Borg is failing with attribute 'three' missing, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-1-lassulus.ewr1.nix.ci/pkgs/applications/science/math/sage/default.nix:65:13

My nix-build -A sage worked when i manually edited the node-packages-v10.nix file, but when i pinned on the json file and ran generate, it changes the attribute name from three to three-0.108.0... I'll take another look...

EDIT: I looked to see how other pinned nodePackages got around this, and it looks like they set an override in default-v10.nix, like fast-cli = nodePackages."fast-cli-1.x".override { ... }

I feel like i'm abusing this pattern somewhat, as i'm not really setting any overrides, but just using this to "rename" the package back to nodePackages.three. cc @svanderburg again to make sure this is appropriate.

A regression in three.js r109 causes the REVISION number to not be set,
breaking the nixpkg for sage

mrdoob/three.js#17342
NixOS#70226
@@ -116,4 +116,6 @@ nodePackages // {
thelounge = nodePackages.thelounge.override {
buildInputs = [ nodePackages.node-pre-gyp ];
};

three = nodePackages."three-0.108.0".override {};
Copy link
Member

Choose a reason for hiding this comment

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

You could probably apply a patch fixing the issue instead of pinning:

three = nodePackages.three.overrideAttrs(old: {
  patches = [ (fetchpatch {url="https://something.com/patch", sha256 = "....";}) ];
})

@prusnak
Copy link
Member

prusnak commented May 30, 2020

Please rework your PR. It now has a merge conflict after PR #89184 has been merged

@SuperSandro2000
Copy link
Member

@jglukasik please wait with reworking this PR after the next big nodePackages update which is happening later today.

@cideM
Copy link
Contributor

cideM commented Jan 9, 2021

@SuperSandro2000 I'm kinda new here out of curiosity which PR are you referring to and can this PR here now move on? If @jglukasik is no longer available I'd pick it up

@SuperSandro2000
Copy link
Member

@SuperSandro2000 I'm kinda new here out of curiosity which PR are you referring to and can this PR here now move on? If

#108235

@jglukasik is no longer available I'd pick it up

sure go ahead.

@stale
Copy link

stale bot commented Jul 9, 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 Jul 9, 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

7 participants