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.

Sign up for GitHub

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

linkchecker: 9.3.1 -> 9.4.0 #65294

Merged
merged 1 commit into from Jul 24, 2019
Merged

linkchecker: 9.3.1 -> 9.4.0 #65294

merged 1 commit into from Jul 24, 2019

Conversation

steshaw
Copy link
Member

@steshaw steshaw commented Jul 23, 2019 •
edited

I'm new to contributing.

It seems I built in a sandbox as the default for useSandbox is true and I haven't disabled it. I tried setting it to true and nix-build didn't rebuild anything. I built using nix-build . -A linkchecker at the top of my nixpkgs checkout. I installed it into my profile with nix-env -f . -iA linkchecker and checked that the new 9.4.0 didn't crash for my use case (it works!).

I ran nix-shell -p nix-review --run "nix-review wip". The exit code was 0.

Closure size checking:

$ git checkout --quiet linkchecker-9.4.0^1 # before
$ nix path-info -S -f . linkchecker
/nix/store/1mxxpbqxj44c10mf8hxr2x84fkv2ghkd-LinkChecker-9.3.1     127870528
$ git checkout --quiet linkchecker-9.4.0
$ nix path-info -S -f . linkchecker
/nix/store/zy2gl0wkaazs58qfcb7c55pyxb147m6d-linkchecker-9.4.0     114048152

Looks like it's smaller. I wrote a little script to help:

$ closure-size
/nix/store/1mxxpbqxj44c10mf8hxr2x84fkv2ghkd-LinkChecker-9.3.1     127870528
/nix/store/zy2gl0wkaazs58qfcb7c55pyxb147m6d-linkchecker-9.4.0     114048152
closure difference = -13822376

No warranties though, the script could be really wrong and overflow etc.

I grepped in nixpkgs didn't reveal any documentation to update. Could be wrong.

I searched for linkchecker in nixos/tests but didn't come up with anything.

Any help appreciated.

Motivation for this change

Version 9.3.1 was crashing.

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.

@ofborg ofborg bot requested a review from peterhoeg July 23, 2019 09:16
peterhoeg
peterhoeg requested changes Jul 23, 2019
View reviewed changes
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
@peterhoeg
Copy link
Member

If I am slow to respond, feel free to ping @worldofpeace who is typically much faster then me.

@ofborg ofborg bot requested a review from peterhoeg July 24, 2019 01:23
@steshaw
Copy link
Member Author

steshaw commented Jul 24, 2019

I believe that I have addressed the items from the code review. Would either @worldofpeace or @peterhoeg take a look? No hurry.

@peterhoeg
Copy link
Member

@FRidh, any thoughts?

worldofpeace
worldofpeace suggested changes Jul 24, 2019
View reviewed changes
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/linkchecker/default.nix Show resolved Hide resolved
@worldofpeace
Copy link
Contributor

Is it possible we could switch to python 3 @steshaw? (in a separate commit)

https://github.com/linkchecker/linkchecker/blob/master/setup.py#L35

@worldofpeace
Copy link
Contributor

I'd also like to mention the issue you had with checking the closure size.

You should do it like

 nix path-info -S -f . linkchecker

-f . does "evaluate FILE rather than the default" (so nixpkgs) then you give it the attribute name.
And not all of the checkboxes need to be checked, it's more of a suggestion of things to do to help the reviewer. (strongly encouraged).

@steshaw
Copy link
Member Author

steshaw commented Jul 24, 2019

@worldofpeace, I tried python 3 while I was working on this and it didn't work. I will try again now that the expression has changed a fair bit.

@steshaw
Copy link
Member Author

steshaw commented Jul 24, 2019

It also looks like I can remove the add-no-robots-flag.patch at https://github.com/NixOS/nixpkgs/tree/9748a91d8197ba911dcc75342372b8a392069979/pkgs/tools/networking/linkchecker

@worldofpeace
Copy link
Contributor

After reading linkchecker/linkchecker#205 I don't think it actually supports python3.

@peterhoeg
Copy link
Member

After reading linkchecker/linkchecker#205 I don't think it actually supports python3.

It's wip: linkchecker/linkchecker#210

@steshaw
Copy link
Member Author

steshaw commented Jul 24, 2019

Early in the review, we changed pythonPath to propagatedBuildInputs. I just noticed that in the previous commit, this change was reversed because a of bad interaction with python3...

It builds either way for me. Just not sure how to check if I'm breaking python3.

@ofborg ofborg bot requested a review from peterhoeg July 24, 2019 06:59
@steshaw steshaw force-pushed the linkchecker-9.4.0 branch 2 times, most recently from 3c50fce to 01acf1a Compare July 24, 2019 07:06
@worldofpeace
Copy link
Contributor

Early in the review, we changed pythonPath to propagatedBuildInputs. I just noticed that in the previous commit, this change was reversed because a of bad interaction with python3...

It builds either way for me. Just not sure how to check if I'm breaking python3.

The motivation for that change was ill informed, #60929 (comment) if I was present I would have reverted it.

@steshaw
Copy link
Member Author

steshaw commented Jul 24, 2019

I've updated the description with how I calculated the closure size difference.

@steshaw
linkchecker: 9.3.1 -> 9.4.0
a452035 
Version 9.3.1 was crashing.
worldofpeace
worldofpeace approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@peterhoeg peterhoeg merged commit b9412f4 into NixOS:master Jul 24, 2019
@peterhoeg
Copy link
Member

Thank you for your contribution @steshaw (and your review @worldofpeace).

Now you know who to ping for your next PR! ;-)

@steshaw steshaw deleted the linkchecker-9.4.0 branch July 27, 2019 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterhoeg peterhoeg peterhoeg approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@steshaw @peterhoeg @worldofpeace