Conversation
ofborg
bot
added
8.has: package (new)
afh
left a comment
afh
left a comment
There was a problem hiding this comment.
Thank you for your time and effort to add a new package to nixpkgs, @Rucadi, much appreciated.
I've left a few comments below that hopefully help to make the package adhere to nixpkgs' current best practices (or rather my limited understanding of it :)
There was a problem hiding this comment.
From what I've picked up finalAttrs is preferred over rec, maybe someone more familiar with the details could chime in and provide some background information in this?
| stdenv.mkDerivation rec { | |
| stdenv.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
Use of the hash keyword is preferred over sha256:
| sha256 = "sha256-pOoqjtFkvMtof+YjV47JPu5tw/U6CU/MpL+C1Z1pgrE="; | |
| hash = "sha256-pOoqjtFkvMtof+YjV47JPu5tw/U6CU/MpL+C1Z1pgrE="; |
There was a problem hiding this comment.
llvmPackages is not referenced anywhere in this package, what am I missing that this is necessary?
There was a problem hiding this comment.
Using makeTarget might be more appropriate here:
| buildPhase = '' | |
| make main | |
| ''; | |
| makeTarget = "main"; |
|
`Thank you for your kind corrections, I'll try to fix everything you mentioned! I checked a video from Tweak that explains finalAttrs https://www.youtube.com/watch?v=jb36PfG28W8 and decided to switch to it :) |
|
Thank you for posting a link to the "Nix Hour #42" video that talks about the benefits of If you've find my suggestions on this PR helpful I'd appreciated if the commit message would include:
Hopefully that's not too much to ask :) |
|
I squashed the commits and included you as a coauthor for the corrections :) |
afh
left a comment
afh
left a comment
There was a problem hiding this comment.
Thank you for accepting my suggestions and crediting me as an co-author, very much appreciated!
Through an unrelated PR of mine (#288767) I became aware of the "superfluous use of pname" just yesterday, hence I'd like to suggest the change to src.repo below.
|
Thanks for chiming in with very helpful advice and providing links to documentation, @kirillrdy, very much appreciated! 🙏 |
|
Thank you all for the suggestions and also the version program naming! I'll for sure remember everything next time 👍 |
|
Result of 1 package built:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1453 |
Rucadi
changed the title
delroth
added
the
12.approvals: 1
|
@Rucadi greetings ! and thank you for your contribution specifically main reason of concern right now is missing license |
I asked @Thomas-de-Bock but he has not given an answer yet, I'll try again to see if that can be clarified. |
There was a problem hiding this comment.
| mkdir -p $out/ | |
| cp -r bin $out/ | |
| install -Dm755 bin/construct -t $out/bin |
|
Result of 1 package built:
|
|
Hello, sorry for not responding here earlier, I've added a license file along with some additional changes to the codebase. |
|
Thank you very much, @Thomas-de-Bock, that's very helpful 🙏 If you feel like also creating a first release¹, that would be helpful from a package maintainer perspective. If not that's alright too as nixpkgs has a package naming/versioning convention for software not providing a version (yet). ———————————— |
|
I've added a release for the linux x86 build. |
|
Thank you, @Thomas-de-Bock, that's truly helpful and very much appreciated! |
|
I'll do it later today when I'm free :) Nice that we have a first version and a license! |
Rucadi
changed the title
delroth
removed
the
12.approvals: 1
Co-authored-by: Alexis Hildebrandt <afh@surryhill.net>
|
Result of 1 package built:
|
|
Thank you for the kind words, the helpful review comments, and merging this PR, @kirillrdy, very much appreciated! 🙏 |
|
Thank you for your words and helping reach the final result, much appreciated :) |
|
Thank you so much to everyone who helped merged, this is super cool! |
7 participants
Description of changes
Construct is a new NASM-like source to source compiler which adds features such as while loops, if statements, scoped macros, simpler function-call syntax and more. It currently supports 64, 32, 16 and 8 bit modes.
It currently only resides on: https://github.com/Thomas-de-Bock/construct but it provides nice abstractions.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.