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

Jump to bottom

lumo 1.9.0 -> 1.10.1 plus darwin support #59414

Merged
merged 2 commits into from Feb 20, 2020
Merged

lumo 1.9.0 -> 1.10.1 plus darwin support #59414

merged 2 commits into from Feb 20, 2020

Conversation

hlolli
Copy link
Member

@hlolli hlolli commented Apr 13, 2019

Motivation for this change

New release, also some requests for adding darwin support which I implemented by adding two missing buildDependencies.

Things done

Removed and added some npm packages, rebuilt the node10 packages files. Tested in on my linux and on MacOs Sierra in VirtualBox.

  • [ x ] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • [ x ] NixOS
    • [ x ] 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"
  • [ x ] 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@hlolli
Copy link
Member Author

hlolli commented Aug 29, 2019

rebased and merge conflict resolved. Can this be merged @markuskowa ?

@matthewbauer matthewbauer requested review from adisbladis, NeQuissimus and svanderburg and removed request for adisbladis and NeQuissimus September 6, 2019 15:57
@anmonteiro anmonteiro mentioned this pull request Dec 10, 2019
Open
@hlolli hlolli mentioned this pull request Feb 7, 2020
Merged
10 tasks
@Mic92
Copy link
Member

Mic92 commented Feb 7, 2020

I will have a look, once it gets updated.

@hlolli hlolli mentioned this pull request Feb 11, 2020
Closed
@raboof
Copy link
Member

raboof commented Feb 11, 2020

I ran into #79781 and can confirm that when using lumo from this branch it does work.

Seems like this PR needs a rebase, though?

@hlolli
Copy link
Member Author

hlolli commented Feb 11, 2020

Because of not being rebased, it will work. I'm finishing work now. I think pkgs.clojure needs a patch or older version provided. Will update in ~2 hours.

@hlolli
Copy link
Member Author

hlolli commented Feb 11, 2020

the branch is rebased now, but and the bug is there. Will try few things out, starting with simplest solutions.

@hlolli
Copy link
Member Author

hlolli commented Feb 11, 2020

The problem seems to be purely due to clojure, adding this check to interpreters/clojure/default.nix

doInstallCheck = true;
  installCheckPhase = ''
   CLJ_CONFIG=`pwd` CLJ_CACHE=$tmp $out/bin/clojure -Spath
  '';

causes

Error building classpath. Failed to read artifact descriptor for org.clojure:clojure:jar:1.10.1
org.eclipse.aether.resolution.ArtifactDescriptorException: Failed to read artifact descriptor for org.clojure:clojure:jar:1.10.1

I think the best way to solve it is by providing the clojure binary an environment where it has access to all its runtime jar deps. This is easy to do, and they do provide deps.edn which clojure reads from on each invocation.

@hlolli
Copy link
Member Author

hlolli commented Feb 11, 2020

after #79868 gets merged, Lumo should work fine, with or without this bump (tough I'd love to see this merged since it's been stale for almost a year).

Mic92
Mic92 reviewed Feb 12, 2020
View reviewed changes
pkgs/development/node-packages/node-env.nix Outdated
@@ -363,7 +354,7 @@ let

npm ${forceOfflineFlag} --nodedir=${nodeSources} ${npmFlags} ${stdenv.lib.optionalString production "--production"} rebuild

if [ "''${dontNpmInstall-}" != "1" ]
if [ "$dontNpmInstall" != "1" ]
Copy link
Member

Choose a reason for hiding this comment

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

This should stay!

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no recollection from this change. Most likely the generated data from 1.6.1. You mean this should stay, the change or the original? (I assume the original, in which case regenerating with right version should fix it).

Copy link
Member

Choose a reason for hiding this comment

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

It is a bug in node2nix itself: svanderburg/node2nix#174

Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the current line.

Mic92
Mic92 reviewed Feb 12, 2020
View reviewed changes
pkgs/development/node-packages/composition-v10.nix Outdated
@@ -1,4 +1,4 @@
# This file has been generated by node2nix 1.7.0. Do not edit!
# This file has been generated by node2nix 1.6.1. Do not edit!
Copy link
Member

Choose a reason for hiding this comment

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

It is weird that this was now regenerated using an older version of node2nix. Did you generated it from an up-to-date nixpkgs repository?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, yes I have older version managed by npm -g, I should ofc install it via nix (but there were some bugs fixed upstream that I needed for something sometime).

Will fix after work

@NeQuissimus NeQuissimus removed their request for review February 12, 2020 13:11
@hlolli
Copy link
Member Author

hlolli commented Feb 19, 2020

@Mic92 I finally got around fixing the comments, the build was also failing, for the same reason the current Lumo build is failing. So I added 2 extra patches and the build goes trough. It would be nice if this could ship with the next release, because as far as I see, Lumo would otherwise get a broken status. I guess for that I'd need to change base?

@raboof
Copy link
Member

raboof commented Feb 20, 2020

branch is still working for me 😄 👍

@Mic92 Mic92 merged commit c385f9f into NixOS:master Feb 20, 2020
@hlolli hlolli deleted the lumo-bump branch February 20, 2020 11:49
@hlolli hlolli mentioned this pull request Feb 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@adisbladis adisbladis Awaiting requested review from adisbladis

@svanderburg svanderburg Awaiting requested review from svanderburg

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) 8.has: package (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@hlolli @Mic92 @raboof @veprbl @GrahamcOfBorg @markuskowa