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

writers: add data-centric writers #244835

Merged
merged 4 commits into from
Jul 25, 2023
Merged

writers: add data-centric writers #244835

merged 4 commits into from
Jul 25, 2023

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Jul 22, 2023

Description of changes

Added some data writers that have a similar interface to the script writers. It's a simple and clean interface that makes the common case nice compared to lib.generators. JSON is being formatter, and YAML is actually YAML, not JSON.

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.

dotnet needs a writable $HOME to operate
@Lassulus
Copy link
Member

looks nice, seems to work. thank you!

@Lassulus
Copy link
Member

not sure why ofborg is unhappy

@zimbatm
Copy link
Member Author

zimbatm commented Jul 22, 2023

Something to do with the test framework expecting derivations instead of recursive attributes. I'll take a look later.

Btw, the pypy and fsharp tests are still broken (that was already true before I started working on this).

For pypy, it looks like pypy2.withPackages is broken.

For fsharp I don't know. I fixed the noDeps test case but there is another issue with nuget.

Make it easier to run and debug individual tests.
Make it easy to write structured data back to disk.
@zimbatm
Copy link
Member Author

zimbatm commented Jul 22, 2023

fixed the CI. Turns out some of the Haskell writers were duplicated ( 73ee03c )

@AndersonTorres
Copy link
Member

I remember a long time ago someone tried to write a writer for a Rust-like serial object notation language.

Does this new approach help?

@Lassulus Lassulus merged commit 17d98b5 into NixOS:master Jul 25, 2023
8 checks passed
@zimbatm zimbatm deleted the data-writers branch July 25, 2023 21:09
@lopsided98
Copy link
Contributor

This breaks writeNginxConfig when cross-compiling, see #245785 for a fix.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

This code is completely redundant and therefore a liability.
Please clean up the duplication.

'';

fish = writeFishBin "test-writers-fish-bin" ''
expectDataEqual = { file, expected }:
Copy link
Member

Choose a reason for hiding this comment

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

You could use testers.testEqualContents instead.

Copy link
Member

Choose a reason for hiding this comment

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

These writers appear redundant, as we already have pkgs.formats for these.
If we're going to keep them, should the respective formats be changed to use these writers?

@roberth
Copy link
Member

roberth commented Oct 16, 2023

JSON is being formatter, and YAML is actually YAML, not JSON.

These issues have long been solved in pkgs.formats.

Comment on lines +12 to +25
# Creates a transformer function that writes input data to disk, transformed
# by both the `input` and `output` arguments.
#
# Type: makeDataWriter :: input -> output -> nameOrPath -> data -> (any -> string) -> string -> string -> any -> derivation
#
# input :: T -> string: function that takes the nix data and returns a string
# output :: string: script that takes the $inputFile and write the result into $out
# nameOrPath :: string: if the name contains a / the files gets written to a sub-folder of $out. The derivation name is the basename of this argument.
# data :: T: the data that will be converted.
#
# Example:
# writeJSON = makeDataWriter { input = builtins.toJSON; output = "cp $inputPath $out"; };
# myConfig = writeJSON "config.json" { hello = "world"; }
#
Copy link
Member

Choose a reason for hiding this comment

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

Where's the user documentation?
This is a new file, but I don't see any code to render this to the manual.

@infinisil
Copy link
Member

Damn, I didn't know about this, indeed this pretty much duplicates pkgs.formats, unfortunate that this is only noticed now 😞

@roberth roberth mentioned this pull request Oct 24, 2023
13 tasks
@roberth
Copy link
Member

roberth commented Oct 24, 2023

Haven't heard anything in over a week.
This took 15 minutes, but 15 minutes too much nonetheless.

Would have appreciated at least some response.

@Mic92
Copy link
Member

Mic92 commented Oct 24, 2023

Maybe this was drowned in below other notifications.

@roberth
Copy link
Member

roberth commented Oct 24, 2023

I'm sure it has. I put a lot of (too much?) effort into at least scanning for notifications where I'm responsible.

@Mic92
Copy link
Member

Mic92 commented Oct 24, 2023

This works until people start mentioning you in more prs than you can read and you don't even get to see the other ones anymore.

@roberth
Copy link
Member

roberth commented Oct 24, 2023

You don't have to read it all to figure out what's relevant, but yeah, maybe I'm putting in too much effort.

@Lassulus
Copy link
Member

I wanted to fix that but the TODO list was already to long. also I'm not super familiar with pkgs.formats (still wondering why it's in pkgs and not lib?) so it would probably have taken more than 15mins for me. thanks for fixing though

@zimbatm
Copy link
Member Author

zimbatm commented Oct 24, 2023

FYI: I saw it and decided to ignore it because of how emotionally charged the messages were. I just don't have the capacity to deal with more drama right now.

@roberth
Copy link
Member

roberth commented Oct 24, 2023

Next time you can tell me. Didn't mean to make drama.

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.

7 participants