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

nixpkgs_package is corrupting binary files #136

Closed
guibou opened this issue Jul 28, 2020 · 4 comments · Fixed by #159
Closed

nixpkgs_package is corrupting binary files #136

guibou opened this issue Jul 28, 2020 · 4 comments · Fixed by #159

Comments

@guibou
Copy link
Contributor

guibou commented Jul 28, 2020

Describe the bug

One of my nixpkgs_package rules loads (in the nix file) a binary file. Which ends corrupted when evaluated by nix in the context of nixpkgs_package (i.e. after the copy).

To Reproduce

Have a binary file directly used in your nix file, something such as:

    src = ./abinaryfile.tar.gz;

This file must appear in the nix_file_deps or your nixpkgs repository. Observes the failure when uncompressing.

Expected behavior

It should not corrupt binaries files.

Environment

  • OS name + version:
  • Version of the code:

Additional context

I tracked the issue to:

https://github.com/tweag/rules_nixpkgs/blob/master/nixpkgs/nixpkgs.bzl#L677

which I extended with the following:

+    print(repository_ctx.execute([repository_ctx.which("sha256sum"), repository_ctx.path(src)]).stdout)
+    print(repository_ctx.execute([repository_ctx.which("sha256sum"), repository_ctx.path(dest)]).stdout)

And indeed observed that the checksum are not the same.

Aside, I'm not convinced by the executable=False flag of the template call. It seems to me that a file which is considered executable must keep its bit. I don't know if that's important for nix however.

It seems that template does not work well with binary files. Unfortunately there is no such things as a simple copy function in repository context.

I tried the following:
- with an execute(["cp", ...]) but I ended with a lot of problems due to how cp behaves with directory versus files or already existing files
- with an symlink, but issues with existing files.

@guibou
Copy link
Contributor Author

guibou commented Jul 28, 2020

I also tried:

    # cp may be called many time, for each rules. So before the symlink,
    # we ensure that the destination does not exists.
    repository_ctx.delete(dest)
    repository_ctx.symlink(src, dest)

It build perfectly fine, however it seems to disable the check for nix_file_deps entirely.

@mboes
Copy link
Member

mboes commented Jul 28, 2020

The fact that repository_ctx.template is not binary-friendly is a nasty gotcha that should be documented upstream. It furthermore strengthens the case for adding a repository_ctx.cp() function.

@mboes
Copy link
Member

mboes commented Jul 28, 2020

@guibou could you open the two tickets upstream as discussed?

Meanwhile, the symlink workaround rendering nix_file_deps is ineffective because Nix might lookup paths inside a .nix file relative to the target of the symlink. In which case we'd be stuck with using the cp command (which we'll have to assume is provided by the host environment).

@guibou
Copy link
Contributor Author

guibou commented Jul 28, 2020

Meanwhile, the symlink workaround rendering nix_file_deps is ineffective because Nix might lookup paths inside a .nix file relative to the target of the symlink. In which case we'd be stuck with using the cp command (which we'll have to assume is provided by the host environment).

I'm having issue with cp, it was creating some directories in subdirectories instead of creating them where they need to appear. I tried cp -T but no success.

@guibou could you open the two tickets upstream as discussed?

guibou added a commit that referenced this issue Mar 21, 2021
This closes #136

`repository_ctx` is meant to work on "text" files and does text encoding
and decoding, which breaks in the context of a binary file.

There is no `cp` API in repository_ctx, as suggested in
bazelbuild/bazel#11858, but
bazelbuild/bazel#11857 suggested to use this
approach with `repository_ctx.file`.
@mergify mergify bot closed this as completed in #159 Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants