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

dhall-json: provide flag -o, --output for writing to file #1303

Closed
DrSensor opened this issue Sep 11, 2019 · 6 comments
Closed

dhall-json: provide flag -o, --output for writing to file #1303

DrSensor opened this issue Sep 11, 2019 · 6 comments
Assignees

Comments

@DrSensor
Copy link

Not having flag -o or --output can cause problems, especially for a project that use docker-compose as their development workflow. For example:

Given this docker-compose.yml

version: "3"
services:
  dhall-json:
    image: dhallhaskell/dhall-json
    entrypoint: [dhall-to-json]
    command: [--file, syntaxes/Dhall.dhall, --pretty]
    volumes:
      - ./syntaxes:/syntaxes:ro

If I run this

docker-compose run --rm --user $(id --user) dhall-json > Dhall.tmLanguage.json

it works but when there is an error, that error will be redirected to Dhall.tmLanguage.json and not printed in stderr.
There is a mode to disable pseudo-tty allocation, but it doesn't solve the problem. For example:

docker-compose run --rm -T --user $(id --user) dhall-json > Dhall.tmLanguage.json

the error is shown in the terminal but when it's succeeded, the results are not written into Dhall.tmLanguage.json.


Also, since the official docker image dhallhaskell/dhall-json is a minimal image (don't provide shell), this docker-compose file will not works:

version: "3"
services:
  dhall-json:
    image: dhallhaskell/dhall-json
    command: [/bin/sh, -c, "dhall-to-json --file syntaxes/Dhall.dhall --pretty > dist/Dhall.tmLanguage.json"]
    volumes:
      - ./dist:/dist:rw
      - ./syntaxes:/syntaxes:ro
$ docker-compose run --rm --user $(id --user) dhall-json
Error response from daemon: OCI runtime create failed: container_linux.go:345: starting container process caused "exec: \"/bin/sh\": stat /bin/sh: no such file or directory": unknown

Though I still prefer the official docker image to be minimal

References

https://docs.docker.com/compose/reference/run/

@sjakobi
Copy link
Collaborator

sjakobi commented Sep 11, 2019

A --output flag seems easy enough to add, but I wonder whether it wouldn't be even easier to fix the docker image to include a shell or some other program that can redirect dhall-json's output to a file?

@Gabriella439
Copy link
Collaborator

It is possible, but my preference is to add an --input and --output flag so that there is no risk of interference from other output over the stdin/stdout channels

However, just in case you're interested in how we'd update the container built by CI, the relevant code is here:

pkgs.dockerTools.buildImage {
inherit name;
contents = [ possibly-static."${name}" ];
};

... and this section of the Nixpkgs manual explains some common things you can do with the buildImage function:

https://nixos.org/nixpkgs/manual/#ssec-pkgs-dockerTools-buildImage

@sjakobi
Copy link
Collaborator

sjakobi commented Sep 11, 2019

It is possible, but my preference is to add an --input and --output flag so that there is no risk of interference from other output over the stdin/stdout channels

The --input flag(s) would replace the existing --file option (which we could keep for compatibility)? And I assume we want this for all the executables in dhall-json?

@Gabriella439
Copy link
Collaborator

@sjakobi: We might not necessarily want to replace --file because part of the reason the flag exists is to set the root directory relative to the file's path

I think in the short term we can just add the --output flag for now as was requested and then figure out the UX for input separately

@sjakobi
Copy link
Collaborator

sjakobi commented Sep 11, 2019

Sounds good. I'll make a patch.

@sjakobi sjakobi self-assigned this Sep 11, 2019
sjakobi added a commit that referenced this issue Sep 11, 2019
mergify bot pushed a commit that referenced this issue Sep 11, 2019
@sjakobi
Copy link
Collaborator

sjakobi commented Sep 12, 2019

Closing this as #1304 addressed the original feature request.

If there's a need for an --input flag as mentioned in #1303 (comment), we can discuss this in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants