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

Some refactoring and fixes in the nix development environment #1827

Merged
merged 6 commits into from
Apr 24, 2021

Conversation

wolfgangwalther
Copy link
Member

@wolfgangwalther wolfgangwalther commented Apr 23, 2021

There are a couple of things I needed to do for #1805, #1812 and while playing around with CI in #1822 - some of which I needed to have fixed in multiple of those PRs. To avoid juggling the same commits in the different PRs, I want to merge them ahead of time.

This PR does the following:

  • Moves default.nix and shell.nix into the nix/ subfolder. This is to have all nix-related files in one directory, which allows to set up the context for a ci-nix-Dockerfile much easier. This adds a convenience wrapper for shell.nix in the root directory, so that nix-shell can still be called the same way as before.
  • Splits cabalTools from devTools to allow installing postgrest-build in CI without the overhead of many devtools dependencies.
  • Fix postgrest-dump-schema currently not working nix-shell --pure. yq needs jq under the hood - but it seems like it currently doesn't import it properly in it's environment. Not sure if that's fixed already upstream, but just adding jq to the PATH fixes it for now.
  • Fix postgrest-coverage needing a rebuild while running the tests, because of changed flags. The build step had --enable-tests all, but the test step did not and so started a rebuild. Now the build targets are listed explicitly and no rebuild is needed.
  • A small improvement to the way we create our withTmpDir temporary directories. They are now created in a common directory /tmp/postgrest/... - this allows to upload all of the kept temp dirs as artifacts in CI in the case of a failed step. Also each individual temp directory is now prefixed with the name of the shell script that was run and not a generic tmp.xxxx. Makes it much easier to debug things.
  • Makes with_tmp_db a true checkedShellScript as discussed in Add postgrest-loadtest based on vegeta #1812 (comment). This also helps with the Dockerfile context (see above). We still need to agree on how we handle the stack situation. The 3 suggestions we have so far are:
    a) Auto-generate a checked-in copy of with_tmp_db
    b) just keep the original file or
    c) create postgrest-stack-build and postgrest-stack-test commands and keep everything in nix.

@monacoremo
Copy link
Member

monacoremo commented Apr 24, 2021

Moves default.nix and shell.nix into the nix/ subfolder. This is to have all nix-related files in one directory, which allows to set up the context for a ci-nix-Dockerfile much easier.

This breaks my workflow, or at least makes it less convenient and a bit suprising to any nix developer. Having the default.nix at the root allows us to run nix-build, nix-build -A postgrestStatic, nix run ..., nix-instantiate (postgrest-push-cachix is broken in this branch right now) in the root of the project without needing to specify a file or directory. I'm very against changing this and giving up the expected behaviour, unless we have an exceedingly strong reason for it. I imagine that it would be possible to create the nix-ci-dockerfile as-is also, maybe with 1-2 extra lines? Can you elaborate on why it is needed?

@monacoremo
Copy link
Member

The 3 suggestions we have so far are:
a) Auto-generate a checked-in copy of with_tmp_db
b) just keep the original file or
c) create postgrest-stack-build and postgrest-stack-test commands and keep everything in nix.

Suggest that we go with b) in this PR.

@wolfgangwalther
Copy link
Member Author

This breaks my workflow, or at least makes it less convenient and a bit suprising to any nix developer. Having the default.nix at the root allows us to run nix-build, nix-build -A postgrestStatic, nix run ..., nix-instantiate (postgrest-push-cachix is broken in this branch right now) in the root of the project without needing to specify a file or directory. I'm very against changing this and giving up the expected behaviour, unless we have an exceedingly strong reason for it. I imagine that it would be possible to create the nix-ci-dockerfile as-is also, maybe with 1-2 extra lines? Can you elaborate on why it is needed?

Love the xkcd :D. You are right, it is not technically needed anymore. I actually wanted to add a question "Remo, I am sure there are problems with this change, what am I missing?" to that part. The background was, that I started with the nix/Dockerfile as-is. To be able to run nix-env -f default.nix -iA ... during the build - all the nix files had to be in the docker file context. I started with just docker build . and those files were out of context. Only much later I realized, that I needed to copy postgrest.cabal, too (to make it install build dependencies with cabal2nix) and would easily be able to change the context directory for the docker command. In my latest setup, I have a checkedShellScript (not pushed anywhere, yet) to create that Dockerfile and the nix/Dockerfile is gone. So... it's not at all inconvenient anymore.

Happy to remove that change :).

@wolfgangwalther
Copy link
Member Author

This breaks my workflow, or at least makes it less convenient and a bit suprising to any nix developer. Having the default.nix at the root allows us to run nix-build, nix-build -A postgrestStatic, nix run ..., nix-instantiate (postgrest-push-cachix is broken in this branch right now) in the root of the project without needing to specify a file or directory. I'm very against changing this and giving up the expected behaviour, unless we have an exceedingly strong reason for it. I imagine that it would be possible to create the nix-ci-dockerfile as-is also, maybe with 1-2 extra lines? Can you elaborate on why it is needed?

Love the xkcd :D. You are right, it is not technically needed anymore. I actually wanted to add a question "Remo, I am sure there are problems with this change, what am I missing?" to that part. The background was, that I started with the nix/Dockerfile as-is. To be able to run nix-env -f default.nix -iA ... during the build - all the nix files had to be in the docker file context. I started with just docker build . and those files were out of context. Only much later I realized, that I needed to copy postgrest.cabal, too (to make it install build dependencies with cabal2nix) and would easily be able to change the context directory for the docker command. In my latest setup, I have a checkedShellScript (not pushed anywhere, yet) to create that Dockerfile and the nix/Dockerfile is gone. So... it's not at all inconvenient anymore.

Happy to remove that change :).

Ah, one more idea:
Not sure if that works, but it should: What about creating just another convenience wrapper similar to shell.nix in the root directory for default.nix? That should still allow the same workflow, but would have all nix code nicely packaged in the nix/ directory. WDYT? What's the up or down side for this?

@monacoremo
Copy link
Member

monacoremo commented Apr 24, 2021

Not sure if that works, but it should: What about creating just another convenience wrapper similar to shell.nix in the root directory for default.nix? That should still allow the same workflow, but would have all nix code nicely packaged in the nix/ directory. WDYT? What's the up or down side for this?

Yes, a file with just `import ./nix' in it should work. I see the value of having everything nix related in the nix directory, but this would also add two extra files, indirection and a bit of extra complexity, I'm not sure it's worth it. How much would it simplify the ci docker?

Edit: Ah, just saw your previous comment! Yes, then I think it's best we leave it as is! :-)

This extends the interface of withTmpDb to allow loading different sets
of database fixtures via `postgrest-with-postgresql-xx --fixtures
<path>`.
This allows to use `postgrest-build` in CI without pulling in a lot of dependencies.
Temporary directories are now created in $TMPDIR/postgrest/script-name-XXX. This allows recognizing the origin of a single temporary directory and uploading the whole $TMPDIR/postgrest folder as an artifact in CI.
@wolfgangwalther
Copy link
Member Author

Kept test/with_tmp_db and kept default.nix and shell.nix in the root directory. Are all other changes good to merge for you, @monacoremo?

@monacoremo
Copy link
Member

Merge time!

@wolfgangwalther wolfgangwalther merged commit c691b37 into PostgREST:main Apr 24, 2021
@wolfgangwalther wolfgangwalther deleted the nix/refactor branch April 24, 2021 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants