Skip to content

writers: add data-centric writers#244835

Merged
Lassulus merged 4 commits intoNixOS:masterfrom
zimbatm:data-writers
Jul 25, 2023
Merged

writers: add data-centric writers#244835
Lassulus merged 4 commits intoNixOS:masterfrom
zimbatm:data-writers

Conversation

@zimbatm
Copy link
Copy Markdown
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
Copy Markdown
Member

looks nice, seems to work. thank you!

@Lassulus
Copy link
Copy Markdown
Member

not sure why ofborg is unhappy

@zimbatm
Copy link
Copy Markdown
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.

zimbatm added 3 commits July 22, 2023 17:53
Make it easier to run and debug individual tests.
Make it easy to write structured data back to disk.
@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Jul 22, 2023
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jul 22, 2023
@zimbatm
Copy link
Copy Markdown
Member Author

zimbatm commented Jul 22, 2023

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

@AndersonTorres
Copy link
Copy Markdown
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
@zimbatm zimbatm deleted the data-writers branch July 25, 2023 21:09
@lopsided98
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Mic92 commented Oct 24, 2023

Maybe this was drowned in below other notifications.

@roberth
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants