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

xeus-zmq: init at 1.1.0 #244775

Merged
merged 1 commit into from
Jul 25, 2023
Merged

xeus-zmq: init at 1.1.0 #244775

merged 1 commit into from
Jul 25, 2023

Conversation

thomasjm
Copy link
Contributor

@thomasjm thomasjm commented Jul 22, 2023

Description of changes

Add the xeus-zmq library.

This is the first step of packaging xeus-cling, the Jupyter kernel for C++. See the draft PR #244777.

Opening this per discussion in the QuantStack/Lobby room. Any chance you could review @serge-sans-paille ?

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

pkgs/development/libraries/xeus-zmq/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/xeus-zmq/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/xeus-zmq/default.nix Outdated Show resolved Hide resolved
Comment on lines 31 to 44
meta = with lib; {
description = "ZeroMQ-based middleware for xeus";
homepage = "https://github.com/jupyter-xeus/xeus-zmq";
maintainers = with maintainers; [ thomasjm ];
platforms = platforms.all;
license = licenses.bsd3;
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
description = "ZeroMQ-based middleware for xeus";
homepage = "https://github.com/jupyter-xeus/xeus-zmq";
maintainers = with maintainers; [ thomasjm ];
platforms = platforms.all;
license = licenses.bsd3;
};
meta = {
description = "ZeroMQ-based middleware for xeus";
homepage = "https://github.com/jupyter-xeus/xeus-zmq";
license = lib.licenses.bsd3;
maintainers = with lib.maintainers; [ thomasjm ];
platforms = lib.platforms.all;
};

Copy link
Contributor Author

@thomasjm thomasjm Jul 22, 2023

Choose a reason for hiding this comment

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

Same question again -- I've gotten the opposite feedback in a prior review, to use a with lib block. Doing some grepping, the way I've done it here seems to be almost universally how the rest of Nixpkgs does it.

Copy link
Member

Choose a reason for hiding this comment

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

This is a bit more technical:

with lib acts like #include <stdlib> in C, bringing all elements of it into the scope and introducing complications on static analysis and many other things.
Also, the nesting of withs brings some nasty inconsistencies.

https://nix.dev/recipes/best-practices#with-scopes

#208242

Copy link
Member

Choose a reason for hiding this comment

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

Both are fine and using with lib here is fine. There is no need to change packages over and it is definitely not something blocking a merge.

@thomasjm
Copy link
Contributor Author

Thanks for the review! I updated the hash, but had a few questions about the style changes.

@thomasjm
Copy link
Contributor Author

Thanks for explaining @AndersonTorres ! I appreciate the review, and I'll make these changes if you insist. But I'd like to lodge a slight protest at this way of effecting change. I've been in this situation before:

  • Someone has an idea for a new best practice for the codebase.
  • A formal style guide or RFC ratifying this best practice has not yet been adopted by the community,
  • But a reviewer wants some new code to adopt the new practice anyway.

This has the effect that the PR gets landed and is "weird" and at odds with the large previously existing codebase. And yet it doesn't actually save any work in the long term, since at some point someone will have to do things the "right way" by doing a full treewide change, presumably by automated means. And who knows, maybe the community will not end up adopting the practice and we'll have introduced divergence into the codebase for nothing.

I say all this without making any comment on the specific merits of the proposals, though of course they're debatable. And I salute efforts to improve the codebase. I just think it's better for new code to favor consistency with existing code and for new practices to be introduced via a proper process.

@AndersonTorres
Copy link
Member

AndersonTorres commented Jul 24, 2023

Without entering the merits of the proposals, your protest would have some merit for "syntatic sugar" parts - namely, things like indentation and one-element-per-line listings. They do not change the AST (abstract syntax tree).

However, with is not a syntatic sugar. It was abused as syntatic sugar, but it isn't.
So, it is far from being amenable to automation.

Indeed, I have given a NixOS-blessed reference about this issue:

https://nix.dev/recipes/best-practices#with-scopes

For completion, here is a more technical post about the hurdles of with:

https://www.tweag.io/blog/2023-01-24-nix-with-with-nickel/

The examples are instructive.

@thomasjm
Copy link
Contributor Author

I definitely don't see why "syntactic sugar"-ness should be a great proxy for "amenable to automation"-ness.

I did not know that nix.dev had become officially blessed, so thanks for the clarification.

I have no doubt that the proposed best practice is a great idea! But that doesn't really change the objection. What good is it for new PRs to avoid relatively innocuous uses of with, so that an experimental new language (Nickel) is easier to write, if the elimination of with doesn't eventually happen tree-wide in Nixpkgs anyway?

Anyway, I've made the changes, thanks for working through this with me :)

@AndersonTorres
Copy link
Member

I definitely don't see why "syntactic sugar"-ness should be a great proxy for "amenable to automation"-ness.

Syntatic sugar is (usually) easier to automate because its evaluation is simpler, not requiring the full power of an evaluator. Sometimes it can be executed via macro substitution (similar to #define in C/C++).

On the other hand, the with construction effectively requires the full power of an evaluator, because it loads potentially arbitrary code.

so that an experimental new language (Nickel) is easier to write,

Nickel is just an example of real world case use. It is not the kernel of the argument.
The text itself cites a thing that is far from being a niche nowadays: Language Server Protocol clients.

What I was pointing out is that with is a nasty keyword, and especially nested withs should be avoided.
(And, incidentally, instances of such nastiness were found in production code.)

I can live with with lib.maintainers; [ chesterton scotus schopenhauer ];, but stacking with lib; { . . . with maintainers; [ . . . ] }; is way more controversial.

if the elimination of with doesn't eventually happen tree-wide in Nixpkgs anyway?

If.

Anyway, I've made the changes, thanks for working through this with me :)

That's fine. This will be useful as a reference in the future.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

editorconfig is now complaining about ghosts trailing whitespaces.

Clean up them and ping me tomorrow to merge.

@thomasjm
Copy link
Contributor Author

Fixed whitespace and squashed, thanks @AndersonTorres !

@SuperSandro2000
Copy link
Member

However, with is not a syntatic sugar. It was abused as syntatic sugar, but it isn't. So, it is far from being amenable to automation.

Indeed, I have given a NixOS-blessed reference about this issue:

nix.dev/recipes/best-practices#with-scopes

For completion, here is a more technical post about the hurdles of with:

tweag.io/blog/2023-01-24-nix-with-with-nickel

The examples are instructive.

Yes but the using with lib; in a limited scope like meta is definitely not a problem. Using something like with pkgs; with qtPackages; with plasma5Packages; over a 500 line file is definitely a problem and has to be improved.

@AndersonTorres
Copy link
Member

Yes but the using with lib; in a limited scope like meta is definitely not a problem.

I am being more radical: with should not be treated as a "factorization trick" like a*b+a*c=a*(b+c).

In this sense, a rule with no margin for exceptions (never use nested with, never use global with) are easy to follow and easy to reason about.
Trying to define "small enough scopes" will eventually blow up because of the vagueness of this concept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants