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

sagittarius-scheme: init at 0.9.6 #65853

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@wahjava
Copy link
Contributor

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
Copy link
Contributor

left a comment

@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/top-level/all-packages.nix Outdated Show resolved Hide resolved

@wahjava wahjava closed this Aug 4, 2019

@wahjava wahjava deleted the wahjava:add/sagittarius-scheme branch Aug 4, 2019

@wahjava

This comment has been minimized.

Copy link
Contributor Author

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:

e551795
ec3e2a9

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Re-opening it :)

@wahjava wahjava reopened this Aug 4, 2019

@aanderse
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

@GrahamcOfBorg build sagittarius-scheme

@aanderse

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

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

@wahjava

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Oh no! The preBuild indentation came back!

@nixos-discourse

This comment has been minimized.

Copy link

commented Aug 13, 2019

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

@@ -85,6 +85,15 @@
github = "baldo";
name = "Andreas Baldeau";
};
abbe = {

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Aug 13, 2019

Member

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?)

This comment has been minimized.

Copy link
@wahjava

wahjava Aug 13, 2019

Author Contributor

Yes, it's.

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

13 checks passed

Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@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
Projects
None yet
4 participants
You can’t perform that action at this time.