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

libucl: init at 0.8.1 #89408

Merged
merged 1 commit into from
Jun 12, 2020
Merged

libucl: init at 0.8.1 #89408

merged 1 commit into from
Jun 12, 2020

Conversation

jpotier
Copy link
Contributor

@jpotier jpotier commented Jun 3, 2020

Motivation for this change

libucl is a dependency for Hikari (#89414)

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.

@jpotier jpotier mentioned this pull request Jun 3, 2020
10 tasks
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
@OPNA2608
Copy link
Contributor

OPNA2608 commented Jun 4, 2020

The library seems to have afew configure options that are set to off be default, maybe you want to look into enabling and adding derivation options for them?

There are

  • optional utilities based on the library
  • optional functionalities like regex / signature checks and url support as well as
  • a version of the library that can be used with Lua.

See configure.ac.

@jpotier
Copy link
Contributor Author

jpotier commented Jun 4, 2020

The library seems to have afew configure options that are set to off be default, maybe you want to look into enabling and adding derivation options for them?

There are

* optional utilities based on the library

* optional functionalities like regex / signature checks and url support as well as

* a version of the library that can be used with Lua.

See configure.ac.

That's a nice pointer, I'll look into this a bit later.

@jpotier
Copy link
Contributor Author

jpotier commented Jun 4, 2020

Signatures support seems out of reach at least for this version of the package: vstakhov/libucl#203

@jpotier jpotier force-pushed the add-libucl branch 2 times, most recently from 6f933d2 to fbac78e Compare June 4, 2020 19:25
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

This will make your future diff after vstakhov/libucl#203 will be resolved even cleaner.

BTW for extra credit, assuming you are fluent with Nix, you could use an attribute set in the inputs instead of those multiple enableThis disableThat flags and use https://nixos.org/nixpkgs/manual/#function-library-lib.strings.enableFeature

pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
@jpotier
Copy link
Contributor Author

jpotier commented Jun 6, 2020

you could use an attribute set in the inputs instead of those multiple enableThis disableThat flags

Do you mean:

{ urls = true;
, regex = false;
, ...}

?

@doronbehar
Copy link
Contributor

Do you mean

No. Maybe start like this:

{ stdenv
, ...
, ...
, features ? {
    urls = true;
    regex = true;
  }
}:

And then when setting configureFlags, you could use https://nixos.org/nixpkgs/manual/#function-library-lib.attrsets.mapAttrs .

, curl
, lua
, openssl
, enableUrls ? false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disable by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reflecting how this is set when you build from source. I'm not familiar with this lib, so I'm assuming all the non default options are extras.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be they are disabled if the necessary deps are not found? It's a common practice to make the build not fail but disable optional features if they are not explicitly enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe yeah. I'm really unsure on what approach to take here. On the other end of the spectrum, enabling everything, seems a bit overkill too.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

@jpotier I'm making these suggestion because I think they are cool but you don't have to go with them. I just sometimes wish to encourage contributors not to afraid of writing nontrivial Nix expressions into packages. I suggest this under the hope that a committer won't object to add such a "complicated" Nix code for a simple library - due to a maintenance reasoning. See e.g: #87940 (comment) .

pkgs/development/libraries/libucl/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libucl/default.nix Show resolved Hide resolved
@jpotier
Copy link
Contributor Author

jpotier commented Jun 6, 2020

@jpotier I'm making these suggestion because I think they are cool but you don't have to go with them. I just sometimes wish to encourage contributors not to afraid of writing nontrivial Nix expressions into packages. I suggest this under the hope that a committer won't object to add such a "complicated" Nix code for a simple library - due to a maintenance reasoning. See e.g: #87940 (comment) .

Oh :/ Too bad your changes weren't merged. I think using the language as much as possible is good, and I definitely like you pushing for these changes I made, and I think they make sense. Of course, these sort of changes should be evaluated on a case-by-case basis.

@doronbehar
Copy link
Contributor

Oh :/ Too bad your changes weren't merged.

Don't worry I got what I wanted at #87949 :).

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

.

@doronbehar
Copy link
Contributor

@GrahamcOfBorg build libucl

@jpotier jpotier requested a review from jtojnar June 7, 2020 09:44
@jpotier
Copy link
Contributor Author

jpotier commented Jun 12, 2020

Alright, I guess this is ready for merging

@jtojnar jtojnar merged commit 3e94344 into NixOS:master Jun 12, 2020
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.

4 participants