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

fix: stop requiring MANIFEST to deduce runfiles directory #937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cerisier
Copy link
Contributor

Addresses #936

@cerisier cerisier changed the title Stop requiring MANIFEST to deduce runfiles directory fix: stop requiring MANIFEST to deduce runfiles directory Sep 14, 2024
@cerisier
Copy link
Contributor Author

cc @fmeum

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are failing

continue

runfiles_dir = _calculate_runfiles_dir(default_info)
# Expect a .runfiles directory for each executable.
runfiles_dir = default_info.files_to_run.executable.short_path + ".runfiles"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may need to be prefixed with ctx.workspace_name.

@cerisier
Copy link
Contributor Author

cerisier commented Sep 16, 2024

Some tests are failing

Tests are failing because it seems that FilesToRunProvider.executable is set for the DefaultInfo of existing files in ctx.attr.srcs (not from generated files tho).

Could this be a Bazel bug ?

@fmeum
Copy link
Member

fmeum commented Sep 16, 2024

Hmm, I will take a look. Since source files can't have runfiles, could you use that check to fix the failure?

@fmeum
Copy link
Member

fmeum commented Sep 16, 2024

On second thought, I don't think that this could be fixed. Bazel needs to set executable since the source file may be executable but it can't stat it at that point to know whether it is.

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

Successfully merging this pull request may close these issues.

2 participants