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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pijul: 0.12.0 -> 1.0.0-alpha #102968

Merged
merged 1 commit into from Nov 11, 2020
Merged

pijul: 0.12.0 -> 1.0.0-alpha #102968

merged 1 commit into from Nov 11, 2020

Conversation

dywedir
Copy link
Member

@dywedir dywedir commented Nov 5, 2020 •
edited

Motivation for this change

馃帀 https://pijul.org/posts/2020-11-07-towards-1.0/

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.

cc @P-E-Meunier

@grahamc
Copy link
Member

grahamc commented Nov 5, 2020

Builds and works from here, thanks! I'm trying to import nixpkgs now :).

cole-h
cole-h approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Diff LGTM. Built with gitImportSupport and was able to successfully import one of my projects into anu.

Can't wait for more docs to show up.

fabianhjr
fabianhjr reviewed Nov 5, 2020
View reviewed changes
pkgs/applications/version-management/anu/default.nix Outdated
Comment on lines 11 to 12
, CoreServices
, Security
Copy link
Member

Choose a reason for hiding this comment

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

These are darwin-specific; might be best to leave them as optional? (Not really sure how this is normally handled, first time I have seen this platform specific dependencies)

Suggested change
, CoreServices
, Security
, CoreServices ? null
, Security ? null

Copy link
Contributor

@omasanori omasanori Nov 5, 2020 •
edited

Choose a reason for hiding this comment

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

Isn't stdenv.lib.optionals stdenv.isDarwin [ CoreServices Security ] sufficient? There are some similar instances in nixpkgs. I'd like to hear from experts whether this is idiomatic or not too, though.

Copy link
Member

Choose a reason for hiding this comment

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

I tried building it on NixOS and had an issue of insufficient arguments. I also don't know how it is normally handled. :C

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Thank you for sharing your situation.

Copy link
Member

Choose a reason for hiding this comment

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

Strange, I built it on NixOS just fine without that...

Copy link
Member

Choose a reason for hiding this comment

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

As reference, I am on NixOS 20.09 with nix-unstable as the user channel. I used nix-build -E "with import <nixpkgs> {}; callPackage ./anu.nix { libclang = llvmPackages.libclang; }" to build this expression and that required the suggested changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to hear from experts whether this is idiomatic or not too, though.

I don't think this is idiomatic, but I'm definitely not an expert.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, passing dependencies in regardless of the platform is the idiomatic approach. We don't reference platform specific dependencies with pkgs.hello instead of passing them in as arguments either.

@andir
Copy link
Member

andir commented Nov 5, 2020

This package is missing manpages. Does upstream have them and we just do not package them?

@cole-h
Copy link
Member

cole-h commented Nov 5, 2020

@andir I don't think there's any but the barest documentation yet. https://anu.dev/documentation/ only has the "Why" and "Getting Started" pages. https://twitter.com/pijul_org/status/1324450002899509253

@P-E-Meunier
Copy link
Contributor

I am totally planning on writing docs. Now that the correctness is more or less there, and that Anu has been tested on a few large-scale repos, and we have a somewhat functional "Nest 1.0", this is now my top priority, and I do have some time to work on it.

@jbaum98
Copy link
Contributor

jbaum98 commented Nov 7, 2020

Just noting here: I've opened a discussion on the appropriate repository on the Anu nest to fix building on aarch64.

@P-E-Meunier
Copy link
Contributor

If I may, I would like to ask you folks to wait a little bit before merging this PR: the new name is not set in stone, and I haven't yet announced it officially.

@dywedir dywedir marked this pull request as draft November 7, 2020 13:08
@dywedir dywedir changed the title anu: init at 1.0.0-alpha pijul: 0.12.0 -> 1.0.0-alpha Nov 9, 2020
@ofborg ofborg bot requested a review from FlorentBecker November 9, 2020 12:26
@P-E-Meunier
Copy link
Contributor

Now you can.

fabianhjr
fabianhjr reviewed Nov 9, 2020
View reviewed changes
Copy link
Member

@fabianhjr fabianhjr left a comment

Choose a reason for hiding this comment

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

It might be best to leave the import of darwin specific libraries as it was implemented on this expression.

(Requiring darwin and getting the specific libraries from darwin)

pkgs/applications/version-management/pijul/default.nix Outdated Show resolved Hide resolved
pkgs/applications/version-management/pijul/default.nix Outdated Show resolved Hide resolved
@dywedir dywedir marked this pull request as ready for review November 9, 2020 19:07
@dywedir dywedir merged commit dd4ebbc into NixOS:master Nov 11, 2020
@dywedir dywedir deleted the anu branch November 11, 2020 16:06
@chpio
Copy link

chpio commented Nov 13, 2020

is the new pijul version able to read repos created with the old/0.12 version? if not then maybe we should keep the old version around?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omasanori omasanori omasanori left review comments

@LnL7 LnL7 LnL7 left review comments

@cole-h cole-h cole-h approved these changes

@fabianhjr fabianhjr fabianhjr approved these changes

@FlorentBecker FlorentBecker Awaiting requested review from FlorentBecker

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@dywedir @grahamc @andir @cole-h @P-E-Meunier @jbaum98 @chpio @omasanori @fabianhjr @LnL7