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

[Bug] tar rule doesn't embed runfiles when --build_runfile_manifests=false #936

Open
cerisier opened this issue Sep 13, 2024 · 2 comments
Open

Comments

@cerisier
Copy link

cerisier commented Sep 13, 2024

When using the tar rule along with --build_runfiles_manifests=false, runfiles are not added to the archive.
I believe this is due to the way the tar rule expects the existence of the MANIFEST to deduce the runfiles directory.

if not default_info.files_to_run.runfiles_manifest:
continue
runfiles_dir = _calculate_runfiles_dir(default_info)

and

if manifest.short_path.endswith("_manifest") or manifest.short_path.endswith("/MANIFEST"):
# Trim last 9 characters, as that's the length in both cases
return manifest.short_path[:-9]
fail("manifest path {} seems malformed".format(manifest.short_path))

The problem is:

  • if using --build_runfiles_manifests=false then there is no MANIFEST or .runfiles_manifest to deduce from.
  • if using --enable_runfiles=false and --build_runfiles_manifests=false then there are no .runfiles directory at all.

I wonder if there is a reliable way to deduce the presence of runfiles, other than checking on existence of MANIFEST file.
And a way to add the runfiles_dir mtree_line only if we know there will be runfiles.

I wonder if one solution wouldn't be to deduce a runfiles_dir from any src[DefaultInfo].default_info.files_to_run.executable and add an mtree_line for it even in the case where there is no runfiles at all.

This would result in having a .runfiles directory for each executable even in the rare event where there are no runfiles at all.

If that's acceptable, I can submit a PR.

@cerisier cerisier changed the title Runfiles are not embedded when --build_runfile_manifests=false [Bug] tar rule doesn't embed runfiles when --build_runfile_manifests=false Sep 14, 2024
@cerisier
Copy link
Author

Maybe @fmeum would know something about that.

@fmeum
Copy link
Member

fmeum commented Sep 14, 2024

I agree that creating a .runfiles directory for each executable is the correct approach. Runfiles are never empty as they always at least contain the executable itself. The logic that inspects runfiles_manifest can be removed.

Happy to review a PR!

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

No branches or pull requests

2 participants