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

sagittarius-scheme: init at 0.9.6 #65853

Merged
merged 2 commits into from Aug 13, 2019
Merged

sagittarius-scheme: init at 0.9.6 #65853

merged 2 commits into from Aug 13, 2019

Conversation

wahjava
Copy link
Contributor

@wahjava wahjava commented Aug 3, 2019

Motivation for this change

Add package for Sagittarius Scheme

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

@wahjava wahjava changed the title sagitarrius-scheme: init at 0.9.6 sagittarius-scheme: init at 0.9.6 Aug 3, 2019
aanderse
aanderse requested changes Aug 4, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

@wahjava Thanks for contributing to nixpkgs and going so far as to adding yourself to the maintainers-list.nix file! We all appreciate your contribution 🎉

I have left some comments I hope you find useful. If you have any questions at all please do not hesitate to ask.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/sagittarius-scheme/default.nix Outdated Show resolved Hide resolved
@wahjava wahjava closed this Aug 4, 2019
@wahjava wahjava deleted the add/sagittarius-scheme branch August 4, 2019 08:11
@wahjava
Copy link
Contributor Author

wahjava commented Aug 4, 2019

Thanks for your advice. I have implemented them except for fetchBitBucket change as mentioned above. While trying to avoid forced commits, I ended up deleting the branch, and re-pushing new commits to a branch with same name, and to my surprise it turns out that resulted in closing this pull request.

Following are the new commits created by me on the branch:

e55179557abf51754f78f91fbaf7cc12099807b7
ec3e2a94038e21f11672d8f2bd7bd8c8217491d8

I wonder if it's possible for you to re-open this pull-request, or you like me to submit a new PR referencing this one.

Sorry for generating more work for you.

@aanderse
Copy link
Member

aanderse commented Aug 4, 2019

Thanks for your work on this. I also seem to be unable to reopen the PR. I wonder if this is the resolution: https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c?

@wahjava
Copy link
Contributor Author

wahjava commented Aug 4, 2019

Re-opening it :)

@wahjava wahjava reopened this Aug 4, 2019
aanderse
aanderse approved these changes Aug 4, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I'll wait for someone with domain specific knowledge to either approve or merge, though. If you don't hear anything on this PR in the next few days ping me and we can actively look for someone.

Thanks again @wahjava!

@aanderse
Copy link
Member

aanderse commented Aug 4, 2019 •
edited

@GrahamcOfBorg build sagittarius-scheme

@aanderse
Copy link
Member

aanderse commented Aug 9, 2019

@wahjava If you're able to fix the darwin build issue that is great, but if not please update platforms accordingly.

@wahjava
Copy link
Contributor Author

wahjava commented Aug 10, 2019

@wahjava If you're able to fix the darwin build issue that is great, but if not please update platforms accordingly.

I can try. I think I know what needs to be fixed, need LD_LIBRARY_PATH equivalent for for Darwin platform. I will ask in channel, if someone can try it on their Darwin builders.

@wahjava
Copy link
Contributor Author

wahjava commented Aug 11, 2019 •
edited

@aanderse pushed new changes in 707c5a4, which I believe should fix the build for darwin platform.

@wahjava
Copy link
Contributor Author

wahjava commented Aug 11, 2019

@aanderse pushed new changes in 707c5a4, which I believe should fix the build for darwin platform.

sorry seems like like a wrong commit on my end 8c32ecd should really fix.

@aanderse
Copy link
Member

Oh no! The preBuild indentation came back!

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/40

7c6f434c
7c6f434c reviewed Aug 13, 2019
View reviewed changes
maintainers/maintainer-list.nix
@@ -85,6 +85,15 @@
github = "baldo";
name = "Andreas Baldeau";
};
abbe = {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious — the maintainer nickname doesn't coincide with anything in the maintainer entry. Should we add some fields to the schema? (Is it your IRC nickname?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's.

@7c6f434c 7c6f434c merged commit 6490f9c into NixOS:master Aug 13, 2019
@7c6f434c
Copy link
Member

@aanderse I have built enough Scheme implementations to judge that domain knowledge is not actually needed for evaluating this expression!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7c6f434c 7c6f434c 7c6f434c left review comments

@aanderse aanderse aanderse approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wahjava @aanderse @nixos-discourse @7c6f434c