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

avy: 2017.10.16 -> cav19 #98239

Merged
merged 1 commit into from Sep 23, 2020
Merged

avy: 2017.10.16 -> cav19 #98239

merged 1 commit into from Sep 23, 2020

Conversation

@Sohalt
Copy link
Contributor

@Sohalt Sohalt commented Sep 18, 2020

Motivation for this change

Update and fix failing build.

ZHF: #97479

Things done

I don't have the domain knowledge to properly test the tool.
Running avy --help and avybmc --help works.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

cc @thoughtpolice

@risicle
Copy link
Contributor

@risicle risicle commented Sep 19, 2020

Builds happily, linux x86_64.

pname = "avy";
version = "2017.10.16";
version = "cav19";

This comment has been minimized.

@rycee

rycee Sep 19, 2020
Member

The version must start with a decimal digit, this will be interpreted as having package name "avy-cav19":

nix-repl> builtins.parseDrvName "avy-cav19"
{ name = "avy-cav19"; version = ""; }

I would suggest continuing with the date based format and put cav19 as a comment, i.e., something like

version = "2020-mm-dd"; # Date of the "cav19" tag.

This comment has been minimized.

@Sohalt

Sohalt Sep 20, 2020
Author Contributor

Thanks for catching that!
You can suggest a code change with the "Insert suggestion" button or hitting ctrl+g by the way. That way I can immediately apply the suggestion from the GitHub interface.

This comment has been minimized.

@risicle

risicle Sep 20, 2020
Contributor

The problem with that way is it leaves us with loads of tiny commits and includes intermediate non-working versions. I find if I'm going to pull that back down and do a squash it's usually just as much work as making the change myself.

This comment has been minimized.

@risicle

risicle Sep 20, 2020
Contributor

And you know what I'm going to ask now ;)

Could you squash your fix in? It just leaves an unnecessarily confusing history.

This comment has been minimized.

@Sohalt

Sohalt Sep 20, 2020
Author Contributor

I was specifically keeping it as a separate commit, to make it easier to see that I've addressed your comment.
Then whoever merges this can just choose "squash & merge".

This comment has been minimized.

@risicle

risicle Sep 20, 2020
Contributor

😬 squash & merge leads to the opposite problem, losing possibly important detail (and bisect-ability) between logically separable changes.

This comment has been minimized.

@Sohalt

Sohalt Sep 20, 2020
Author Contributor

But not for something this small, where my manual squash would end up with a single commit anyway. I'm all for logically structured commits for larger changes.
But my preferred workflow is usually:

  • initial version of PR
  • address comments in individual commits
  • rebase/squash right before merging (if multiple commits are needed do it manually, if one is sufficient, just use the GitHub UI)

Though I'm not going to argue much over such detail. I've squashed & pushed.

@Sohalt Sohalt force-pushed the Sohalt:avy branch from 6353761 to 4072234 Sep 20, 2020
Copy link
Member

@thoughtpolice thoughtpolice left a comment

Thanks!

@thoughtpolice thoughtpolice merged commit daaa0e3 into NixOS:master Sep 23, 2020
19 checks passed
19 checks passed
tests tests
Details
action
Details
avy, avy.passthru.tests on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
avy, avy.passthru.tests on x86_64-linux Success
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="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="4072234"; rev="40722346b81d28e9040a38ba44df6e49dbc80d3e"; } ./pkgs/t
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
@Sohalt Sohalt mentioned this pull request Sep 24, 2020
4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.