-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
hobbes: init at latest #79699
hobbes: init at latest #79699
Conversation
Looking into the build timeout. It passes on Travis CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that all LGTM.
What else can I do to help with this PR? Do I need to tick all the boxes on the template list? |
You can try and write at https://discourse.nixos.org/t/prs-already-reviewed/2617 . I don't have merge permissions so I've done as best as I could. Since I can see this is your first contribution, 1st of all welcome aboard :) And 2ndly, get used to it - PRs can take a longgg of time. |
Thanks @doronbehar! |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Building with latest GCC and LLVM fails. There are open issues on upstream for that.
I'll fix it later today when I have a chance and let you know. Thank you all for guiding me on this PR. |
@thmzlt awesome! Links to upstream issues always appreciated so future updates can easily check if these issues are resolved or not. |
Pushed changes to set Python as a check phase input. Should be good to go. Regarding the issues related to LLVM/GCC versions, they are:
Both issues are expected to be fixed. |
Cool, thanks!
Maybe it's worthwhile adding a short comment to the derivation above the respective dependencies with the links? Avoid that somebody has to hunt for PRs in the future to find these links ;). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice there are a few unresolved comments from other reviewers.
@aanderse, just pushed a new version of the derivation with your feedback. Let me know if there's anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good! Very close, a few more changes, a quick test, and I'm more than happy to merge. Thanks for your patience up to this point.
@aanderse, I have fixed the version name, and separated the changes in two commits. I have also changed the maintiner from the upstream's maintainer to myself since I'm the one working on the derivation. Finally, I have added a comment pointing to the upstream issue on why doCheck is currently set to false, and removed the python37 input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thmzlt what a fantastic job you've done here. Welcome to the maintainers team, we're so glad to have you! 🎉
@GrahamcOfBorg build hobbes |
Oh no! 😱 |
I can't test it on Darwin at the moment (I don't have a supported computer). Is it possible to mark this as Linux-only while I figure out how to build on a Mac? |
Yes. If upstream intended this to build on darwin, then we mark this as broken there. I.e use If upstream intended this to work only on Linux, you can use the |
@GrahamcOfBorg build hobbes |
Looks like we are good. Thank you all for the help. |
hobbes: init at latest (cherry picked from commit 76b292d)
Motivation for this change
Introducing the hobbes programming and execution environment build and maintained at Morgan Stanley.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)