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

nixpack plan generates an invalid toml file for python projects containing a .tool-versions file. #1241

Closed
1 task done
Nevsor opened this issue Dec 13, 2024 · 6 comments · Fixed by #1249
Closed
1 task done
Labels
bug Something isn't working

Comments

@Nevsor
Copy link
Contributor

Nevsor commented Dec 13, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When running nixpack plan -f toml in a directory containing a Poetry project and a .tool-versions file nixpacks outputs a line before it outputs the .toml-file:

Using poetry version from .tool-versions: 1.8.5

This breaks the deployment code in Coolify, because Coolify will read the output and try to parse it as toml.
I expect the same problem to occur with the json output format.

To reproduce

  1. Create a Poetry project or simulate this with touch pyproject.toml poetry.lock
  2. echo "poetry 1.8.5" > .tool-versions
  3. nixpacks plan -f toml .

Expected behavior

I would expect nixpacks to output just the toml file of the build plan.

Environment

On the host:

$ docker -v
Docker version 27.0.3, build 7d4bcd8

In the container:

cat /etc/os-release
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.20.3
PRETTY_NAME="Alpine Linux v3.20"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues"
$ nixpacks --version
nixpacks 1.29.0
@Nevsor Nevsor added the bug Something isn't working label Dec 13, 2024
Nevsor added a commit to Nevsor/nixpacks that referenced this issue Dec 13, 2024
…python projects containing a .tool-versions file.

I assume that outputting those lines before the toml or json file is not intended. If it is then please ignore this commit.

Another option might be to print these messages to stderr instead?
@CoderJoshDK
Copy link
Contributor

To me, it seems like the behavior of printing out the output as it does, is correct in normal cases. Rather, you are really saying that if the toml flag is present, the output should be only toml. So instead of removing the println, maybe it is better to output it only on specific output formats? That seems like an overly complex refactoring to make possible. So instead, either Coolify should fix this bug (since this is more a coolify bug) or, maybe a new flag is added that lets you output the plan to a file and have coolify parse that file (this is much easier logic to add in and I could see other use cases of this feature.)

@Nevsor
Copy link
Contributor Author

Nevsor commented Dec 22, 2024

I don't think the format is important here. I thought the toml flag would cause the command to output just the toml file and the default json flag to output just the json file.

I believe that a command line utility is more useful if it is easy to intergrate into other systems. When choosing between behaviour A "the command outputs a file in json/toml format" or behaviour B "the command outputs a file in json/toml format, but occasionally also outputs a few lines of english text", the first option seems more intuitive, easier to use and less error prone.

With behaviour A a user can just

  • write a shell script that pipes the output of nixpacks plan . to a file (like this blogpost thinks is possible using nixpacks plan . -f toml > nixpacks.toml ) or
  • call the command from another process and parse its output as json/toml (like Coolify does).

With behaviour B every user that tries to use the output of nixpacks plan in a program also has to:

  1. Figure out, that the command sometimes prints messages before the json/toml. Something that is not yet documented here: https://nixpacks.com/docs/cli#plan
  2. Implement some logic to detect where in the output the json/toml file starts. This does not seem completely trivial to me. Do I just ignore every line until I find a line containing an '=' sign? How to I verify that nixpacks won't output a message containing '='? Is it possible for messages to be printed after the file?

Of course Coolify could implement something like this and as a Coolify user I am happy as long as my deploys work again, but I believe that making nixpack plans output easy to parse would benefit more use cases and would prevent bugs in other downstream applications.

How would you feel about changing the println! calls to eprintln! instead of deleting them? This would follow the command line convention of separating a command's result (stdout) and messages to the user (stderr). A user would then be able to easily use the output like nixpacks plan . > nixpacks.json (write the plan to a file), nixpacks plan . | jq . (print plan with syntax highlighting) or nixpacks plan . | jq variables.NODE_ENV | grep production > /dev/null && other_command (execute other_command if NODE_ENV="production"). In addition the user would still be able to see the Using poetry version from .tool-versions: 1.8.5 and similar messages, because those won't be redirected.

When comparing this to the solution of adding a new flag to write the plan to a file, this might require changing more parts of the code and is in case any breaking change if any downstream application actually relies on parsing those messages. On the other hand being able to directly use the output

  • simplifies the code of every downstream application.
  • does not require a writable filesystem.
  • prevents downstream applications from erroneously assuming they can parse the output as json/toml. This assumption is dangerous, because its mostly correct, but not always. Also the docs lead us to believe its true.

You could also consider to print something like "Build plan:" before outputting the json/toml. This would make it easier for downstream applications to find the start of the plan file, but this would surely break some downstream applications and is still more complicated than just capturing the output and passing it to a json/toml parser.

@Nevsor
Copy link
Contributor Author

Nevsor commented Dec 22, 2024

I looked at some projects, that rely on nixpacks. Most don't use nixpacks plan at all, but the only one out of my sample that does, also assumes, that it can pass the output into a json parser without further filtering:

https://github.com/Vano2903/nixpacks-go/blob/019906b3a1cbe97acfde2277dba14b8f969f338b/plan.go#L35-L47

Also none of the tutorials and blogposts I found show anything but the json as output or mention that there might be some other output before it, which makes it very easy for other projects trying to use the command to produce the same bug.

@CoderJoshDK
Copy link
Contributor

You are right. The python provider is very inconsistent here. Other providers don't seem to have this problem (technically, there are other providers that throw up warning but not errors into println. But more on that in a sec). In part, this is due to the python ecosystem. But regardless of that, you are right. The command should just output the raw valid parsable output. I would still love a way to get that info for debug reasons (since versions can be defined in so many different files ...)

Making the debug info go to stderr isn't a great solution, since some other programs process any stderr as a failure of some type. Or at the very least, they complain about it. So that change makes sense for the other providers that have warnings but just provide default values. Regardless of that, I think you are right that the default behavior of that plan flag should just generate the parsable plan. The other stuff is just implementation details.

@Nevsor
Copy link
Contributor Author

Nevsor commented Dec 23, 2024

Thanks for the insight regarding the other providers and the programs interpreting stdout as an error. Maybe a command line option (--verbose or --quiet depending on which default behaviour you prefer), that determines whether to print additional information to stderr, would be nice to have.

@CoderJoshDK
Copy link
Contributor

I created a PR that cleans up more offenders. I ended up skipping the --verbose or --quiet flags due to the non-trivial amount of refactoring required. If there ends up being a larger, important reason why we actually do need a verbose mode, it should be revisited then. But in the more immediate route, IMO, we should get your actual problem fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants