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

git-big-picture: 1.0.0 -> 1.1.1 #110201

Merged
merged 1 commit into from Jan 31, 2021
Merged

git-big-picture: 1.0.0 -> 1.1.1 #110201

merged 1 commit into from Jan 31, 2021

Conversation

hartwork
Copy link
Contributor

  • Migrate from fetchFromGitHub to fetchPypi to ease SHA256 handling
  • Integrate test suite
  • Install a man page (as shipped by upstream)
Motivation for this change
Things done
  • 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.

@hartwork
Copy link
Contributor Author

@SuperSandro2000 I think I applied your suggestions 1:1 but the build still fails. Any ideas why?

@SuperSandro2000
Copy link
Member

You are not in a python package and cannot directly use python packages in inputs. You need to use with python3Packages; [ cram ] instead.

@hartwork
Copy link
Contributor Author

You are not in a python package and cannot directly use python packages in inputs. You need to use with python3Packages; [ cram ] instead.

Interesting, thanks! I guess that applies to pytest as well? Let's see if the CI likes the new version.

@ofborg ofborg bot requested a review from nthorne January 24, 2021 01:20
@hartwork hartwork force-pushed the update-git-big-picture branch 2 times, most recently from 01ad52e to 3708ccd Compare January 24, 2021 02:04
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 24, 2021 •
edited

I guess that applies to pytest as well?

Yes, it does.

Please do not use ofborg to test evaluation. This needs to be done locally or ofborg would be to busy for the important evals.

@hartwork
Copy link
Contributor Author

hartwork commented Jan 24, 2021 •
edited

Please do not use ofborg to test evaluation. This needs to be done locally or ofborg would be to busy for the important evals.

@SuperSandro2000 I understand and respect your concern.

With my prior update to 1.0.0, getting the security fix to the users was my motivation to try bump the package while no one else did the bump before me: We probably agree that keeping nixOS users safe and secure is important.
With 1.1.1 here, the situation is a bit different: The bump re-integrates the test suite and gets the manpage to users. I did it because the previous bump to 1.0.0 made me a bit more familiar with nixOS and because things seemed to go fastest if I get involved myself once more. We probably agree that these changes are not as important as security fixes.

Given that I am not running nixOS anywhere myself, ofborg is my only ship to get security fixes into nixOS. Can we agree that security work is important enough to use ofborg resources and that regular bumps are not? That would be fair and good with me.

@hartwork
Copy link
Contributor Author

@SuperSandro2000 given what you said about ofborg above: Should I try finish the 1.1.1 bump still or do you know anyone who could take over? With the most recent change, I have a feeling it may be the last round.

@SuperSandro2000
Copy link
Member

Nothing wrong with updating packages or force pushing but you can check your builds locally with nix and enabled sandbox on almost any platform and you can also run a nixpkgs-review wip to check for regressions.

@hartwork
Copy link
Contributor Author

Nothing wrong with updating packages or force pushing but you can check your builds locally with nix and enabled sandbox on almost any platform and you can also run a nixpkgs-review wip to check for regressions.

@SuperSandro2000 those would all be firsts to me so I'm afraid that's out of my time budget box right now.
The logs indicate that the PyPI files do not contain the test suite (which makes sense given their location). So we'll need to go back to fetching from GitHub with a bas32 checksum that you told me I cannot generate outside of nixOS and with mutliple iterations of ofborg more ahead. I think we're at a point now where I need someone to take over until I have more experience with nixOS myself. Should I close this PR?

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110201 run on x86_64-linux 1

1 package failed to build and are new build failure:

@SuperSandro2000
Copy link
Member

GitHub with a bas32 checksum that you told me I cannot generate outside of nixOS

That is not correct. You can use nix which you can use on any other Linux distribution.

with mutliple iterations of ofborg more ahead

Please do not use ofborg to generate checksums. It is not meant for this.

@nthorne could you maintain this package? If not we should think about removing it completely.

@infinisil
Copy link
Member

Turns out that a wheel distribution doesn't include the tests, so we can't run them. I force pushed a commit that does that, and also includes a small cleanup. I hope you don't mind, @hartwork :)

@hartwork
Copy link
Contributor Author

@infinisil thanks for your help!
@SuperSandro2000 CI is green thanks to @infinisil — ready to merge?

@nthorne
Copy link
Contributor

nthorne commented Jan 25, 2021

Hi,

Really sorry for jumping onto this so late, @SuperSandro2000 , but things are a bit busy elsewhere. Yes, I can maintain this package, but it will have to be prioritized with regards to other commitments.

@hartwork @infinisil
git-big-picture: 1.0.0 -> 1.1.1
201b24a 
- Migrate from fetchFromGitHub to fetchPypi to ease SHA256 handling
- Drop tests (while not included with PyPI releases)
- Install a man page (as shipped by upstream)
- Move buildInputs to runtime only

Co-Authored-By: Silvan Mosberger <contact@infinisil.com>
@hartwork
Copy link
Contributor Author

@SuperSandro2000 is anything important left to fix before a merge?

@infinisil infinisil merged commit ba6292f into NixOS:master Jan 31, 2021
@hartwork hartwork deleted the update-git-big-picture branch August 26, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@nthorne nthorne Awaiting requested review from nthorne

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@hartwork @SuperSandro2000 @infinisil @nthorne