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

nfpm(bugfix): handle package targets w/ output_path #21806

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jan 2, 2025

A package target with an output_path puts the packaged file relative to the build_root instead of relative to the BUILD file. So, we need to explicitly check for both the file_path (relative to BUILD file) and the value (which might be relative to the build_root) instead of just the file_path.

In debugging this, I ended up copying the plugin code into a separate repo so I could figure out what was going wrong. It would have been easier to debug if the sandbox contents were listed in the error message that claims the src file was missing. So, this also expands the error messages so future issues can be identified (hopefully) more simply.

A package target with an output_path puts the packaged file
relative to the build_root instead of relative to the BUILD
file. So, we need to explicitly check for both the file_path
(relative to BUILD file) and the value (which might be
relative to the build_root) instead of just the file_path.
Debugging missing files is very difficult because the sandbox cannot be
inspected until pants creates a process sandbox. The src is missing
error happens before running nfpm, however, so there is no sandbox to
"preserve on_failure". So, the error message also includes the list of
sandbox files now.
@cognifloyd cognifloyd requested a review from a team January 2, 2025 19:49
@cognifloyd cognifloyd self-assigned this Jan 2, 2025
@cognifloyd cognifloyd modified the milestones: 2.24.x, 2.23.x Jan 2, 2025
@cognifloyd
Copy link
Member Author

cognifloyd commented Jan 2, 2025

I put this in the 2.23.x milestone because the nfpm backend was first released in 2.23.

I also skipped release notes.

@cognifloyd cognifloyd added the release-notes:not-required PR doesn't require mention in release notes label Jan 2, 2025
@cognifloyd cognifloyd enabled auto-merge (squash) January 2, 2025 22:17
@cognifloyd cognifloyd merged commit f70f715 into main Jan 2, 2025
24 checks passed
@cognifloyd cognifloyd deleted the cognifloyd/nfpm-src-bugfix branch January 2, 2025 22:36
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.23.x

Successfully opened #21808.

✔️ 2.24.x

Successfully opened #21809.


Thanks again for your contributions!

🤖 Beep Boop here's my run link

cognifloyd added a commit that referenced this pull request Jan 2, 2025
…21806) (#21809)

A package target with an output_path puts the packaged file relative to
the build_root instead of relative to the BUILD file. So, we need to
explicitly check for both the file_path (relative to BUILD file) and the
value (which might be relative to the build_root) instead of just the
file_path.

In debugging this, I ended up copying the plugin code into a separate
repo so I could figure out what was going wrong. It would have been
easier to debug if the sandbox contents were listed in the error message
that claims the src file was missing. So, this also expands the error
messages so future issues can be identified (hopefully) more simply.

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
cognifloyd added a commit that referenced this pull request Jan 2, 2025
…21806) (#21808)

A package target with an output_path puts the packaged file relative to
the build_root instead of relative to the BUILD file. So, we need to
explicitly check for both the file_path (relative to BUILD file) and the
value (which might be relative to the build_root) instead of just the
file_path.

In debugging this, I ended up copying the plugin code into a separate
repo so I could figure out what was going wrong. It would have been
easier to debug if the sandbox contents were listed in the error message
that claims the src file was missing. So, this also expands the error
messages so future issues can be identified (hopefully) more simply.

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants