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

Make the file generation more transparent #203

Merged
merged 8 commits into from
Jun 26, 2024
Merged

Conversation

thufschmitt
Copy link
Member

Regenerate the files (defined in files.*) automatically when entering the shell (if needed).

Also unify that mechanism with the one used to generate the lockfile so that everything is generated at once.

The lockfile generation is slightly hacky because there's no easy way to pass its content from Nix to Nickel. But that'll have to do the job.

enrichedFlakeInputs =
flakeInputs
// {
"%%organist_internal".nickelLock = builtins.toFile "nickel.lock.ncl" (buildLockFileContents lockFileContents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be nice to add a little comment explaining why we need to do that, for future you, me and others

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Has the doc been lost in a force push?I can't see it still

run-test.sh Outdated
@@ -120,7 +119,7 @@ test_example () (
cp -r "$examplePath" ./example
pushd ./example
prepare_shell
nix run .\#regenerate-files --print-build-logs
# nix run .\#regenerate-files --print-build-logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line still there on purpose or should it just be scraped entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, that's purely accidental. Removed it

@thufschmitt
Copy link
Member Author

It turns out that this blows-up the Nickel evaluation time (~x30 according to my benchmarks). I'll have to investigate why before we can merge this

@thufschmitt thufschmitt marked this pull request as draft May 29, 2024 07:11
@yannham
Copy link
Contributor

yannham commented May 29, 2024

x30 for such a change is really suspicious...

@thufschmitt
Copy link
Member Author

x30 for such a change is really suspicious...

tweag/nickel#1930 seems to be the culprit (possibly also the real culprit for tweag/nickel#1630)

@thufschmitt thufschmitt changed the base branch from main to only-export-flake June 17, 2024 12:06
@thufschmitt
Copy link
Member Author

Rebased on top of #205 . Somehow, this sidesteps the performance issue 🥳

@thufschmitt thufschmitt marked this pull request as ready for review June 17, 2024 13:09
@thufschmitt thufschmitt force-pushed the transparent-regen branch 2 times, most recently from 26beeda to 43dd787 Compare June 19, 2024 11:28
Base automatically changed from only-export-flake to modules June 19, 2024 16:38
Base automatically changed from modules to main June 20, 2024 16:14
@thufschmitt thufschmitt requested a review from yannham June 25, 2024 08:59
@thufschmitt
Copy link
Member Author

(Note that this accidentally includes #210. I can revert it if you need @yannham )

else
chmod +w sources
fi
cp $expectedLockfileContentsPath sources/nickel.lock.ncl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cp $expectedLockfileContentsPath sources/nickel.lock.ncl
cp "$expectedLockfileContentsPath" sources/nickel.lock.ncl

lib/files.ncl Outdated Show resolved Hide resolved
lib/files.ncl Outdated Show resolved Hide resolved
lib/files.ncl Outdated Show resolved Hide resolved
enrichedFlakeInputs =
flakeInputs
// {
"%%organist_internal".nickelLock = builtins.toFile "nickel.lock.ncl" (buildLockFileContents lockFileContents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Has the doc been lost in a force push?I can't see it still

Théophane Hufschmitt added 2 commits June 26, 2024 17:49
Removes the need from running `regenerate-files` manually
This is in particular useful when the source file is generated with Nix.
Use for instance with:

```nickel
{
    files."foo".file = nix.import_nix "nixpkgs#foo",
}
```
Avoids the need for an explicit `nix run .#regenerate-lockfile`
Théophane Hufschmitt and others added 4 commits June 26, 2024 18:03
Ensure that autocompletion works fine for simple things
Ooops

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
Got bitten by tweag/topiary#654 once again

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
@thufschmitt thufschmitt added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit 5ec9c0b Jun 26, 2024
1 check passed
@thufschmitt thufschmitt deleted the transparent-regen branch June 26, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants