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
dasm: init at 2.20.13 #84739
dasm: init at 2.20.13 #84739
Conversation
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.
Final couple requested changes:
It would be nice if you could 1) squash your commits back into the original init at
commit, and 2) separate your maintainer changes into a separate commit: maintainers: add jwatt
.
Thanks for bearing with my nitpicks.
maintainers/maintainer-list.nix
Outdated
jwatt = { | ||
email = "jwatt@broken.watch"; | ||
github = "jjwatt"; | ||
githubId = "2397327"; |
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.
githubId = "2397327"; | |
githubId = 2397327; |
Just to fall in line with the rest of the file ;)
Didn't know that I could do that in a review/pull-request! I appreciate your time and feedback! I don't consider them nits. I haven't worked this deeply with github before and am used to gerrit, plus I'm new to nix committing, so this is a learning experience. er, I assume by splitting and squashing you mean with a good ol' git rebase. I'll try that and push it and see if it works. |
That seemed to work, but I somehow lost the use of version var change that you suggested. Lemme add that back in. |
@@ -0,0 +1,33 @@ | |||
{stdenv, fetchFromGitHub}: |
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.
You don't consider my suggestions nits? Well then, here's a nit for ya:
{stdenv, fetchFromGitHub}: | |
{ stdenv, fetchFromGitHub }: |
(please :) )
Sorry, I lied when I said last couple changes above :)
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.
lol. Nope! bring it on :)
ahh. I just discovered nixfmt
, too.
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.
Yeah, nixfmt is cool but it's not official yet. I.e. it's not the standard for nixpkgs.
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.
LGT my untrained eye! Even ran a little test program and it compiles/runs fine. Should be good to go, assuming nobody has more nits! Though it might take a bit to get merged; be patient, bug somebody on IRC, or post in the Discourse thread for reviewed PRs: https://discourse.nixos.org/t/prs-already-reviewed/2617
[3 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/84739
32 package marked as broken and skipped:
clang-sierraHack digitalbitbox gdata-sharp iterm2 linuxPackages_4_4.evdi linuxPackages_hardkernel_4_
14.bcc linuxPackages_hardkernel_4_14.bpftrace linuxPackages_hardkernel_4_14.can-isotp linuxPackages_
hardkernel_4_14.chipsec linuxPackages_hardkernel_4_14.digimend linuxPackages_hardkernel_4_14.evdi li
nuxPackages_hardkernel_4_14.mba6x_bl linuxPackages_hardkernel_4_14.nvidia_x11 linuxPackages_hardkern
el_4_14.nvidia_x11_legacy390 linuxPackages_hardkernel_4_14.prl-tools nix-linter octave-jit octoprint
php73Extensions.zmq php74Extensions.couchbase php74Extensions.pcs php74Extensions.pthreads php74Ext
ensions.zmq python27Packages.caffe python27Packages.habanero python27Packages.handout python27Packag
es.hass-nabucasa python37Packages.nixpart python38Packages.libselinux python38Packages.nixpart pytho
n38Packages.poezio python38Packages.pyblock
1 package built:
dasm
Test program:
str set "Hello Nix!"
echo str
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.
Test program:
echo str
Can confirm that it works!
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.
The test directory and make target are called test
, not tests
. If you also add doCheck
, nix will run the tests :)
Please also use 2 spaces for indentation instead of tabs.
Co-authored-by: Daniel Schaefer <git@danielschaefer.me>
Co-authored-by: Daniel Schaefer <git@danielschaefer.me>
It looks like it is checked on my end. |
I squashed the commits and pushed them as fbb76f6. Thanks for the contribution! |
Motivation for this change
Add the "dasm" assembler to nixpkgs.
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)