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.

Sign up for GitHub

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

Jump to bottom

executor: allow capturing error message from scriptlets #966

Merged
merged 16 commits into from
Jan 18, 2025
Merged

executor: allow capturing error message from scriptlets #966

merged 16 commits into from
Jan 18, 2025

Conversation

cmatsuoka
Copy link
Collaborator

@cmatsuoka cmatsuoka commented Jan 10, 2025 •
edited
Loading

Change craftctl communication channel from named pipes to unix
sockets to consolidate the ctl server and error stream capture.

Process execution runs a selector to obtain data from i/o streams
to obtain the isolated error output. Allow this selection loop run
on a caller-specified selector with handlers registered to also
process interactions with craftctl. The captured error output when
executing scriptlets is passed to ScriptletRunError. Errors from
craftctl default execution in the build step raise PluginBuildError.
This is consistent with the exception raised by the built-in build
step handler.

Signed-off-by: Claudio Matsuoka claudio.matsuoka@canonical.com

  • Have you signed the CLA?
  • Have you added an entry to the changelog (docs/reference/changelog.rst)?

Notes to reviewers: see individual commits for pipe-to-socket change, and socket selector consolidation.

cmatsuoka
cmatsuoka commented Jan 10, 2025
View reviewed changes
craft_parts/errors.py Outdated Show resolved Hide resolved
@cmatsuoka cmatsuoka force-pushed the CRAFT-3828-Capture-error-in-step-scriptlet-execution branch 14 times, most recently from 4c9eb16 to 5e42241 Compare January 11, 2025 22:37
@cmatsuoka cmatsuoka marked this pull request as ready for review January 12, 2025 16:32
@cmatsuoka cmatsuoka requested review from tigarmo, mr-cal and bepri January 12, 2025 16:32
bepri
bepri requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@bepri bepri left a comment

Choose a reason for hiding this comment

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

I think the functionality looks good - the sockets solution is nice. Just a few small things. Thanks!!

craft_parts/ctl.py Show resolved Hide resolved
craft_parts/executor/step_handler.py Show resolved Hide resolved
craft_parts/utils/process.py Show resolved Hide resolved
craft_parts/errors.py Outdated Show resolved Hide resolved
tests/unit/executor/test_step_handler.py Outdated Show resolved Hide resolved
craft_parts/ctl.py Show resolved Hide resolved
craft_parts/executor/step_handler.py Show resolved Hide resolved
tigarmo
tigarmo reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

@cmatsuoka have you run snapcraft's spread tests with these changes? I think we should

tests/unit/executor/test_step_handler.py Show resolved Hide resolved
@cmatsuoka
Copy link
Collaborator Author

@cmatsuoka have you run snapcraft's spread tests with these changes? I think we should

I'll prepare a snapcraft PR to to so.

@cmatsuoka cmatsuoka requested a review from bepri January 14, 2025 15:13
lengau
lengau reviewed Jan 17, 2025
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@cmatsuoka cmatsuoka requested review from lengau and tigarmo January 17, 2025 16:18
@cmatsuoka
ctl: change craftctl channel to socket
ed780e4 
Change to craftctl communication channel from named pipes to unix
sockets to make it easier to consolidate the ctl server and error
stream capture.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
process: allow using process selector with custom handlers
a384ef3 
Process execution runs a selector to obtain data from i/o streams
to obtain the isolated error output. Allow this selection loop run
on a caller-specified selector with handlers registered to also
process interactions with craftctl.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
docs: update changelog
67fe394 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
tests: add unit tests
b6cdf5b 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
chore: address linter warnings
0da3469 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
executor: refactor scriptlet environment setting
e338d83 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
tests: refactor ctl test
d146ac9 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
chore: add word to spelling list
c7683c1 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
tests: craftctl default raises PluginBuildError
4fcc037 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
chore: change scriptlet environment script name
865351e 
Step execution environment is step-specific, and the existing
environment.sh script is for the build step only. Rename the file
containing the scriptlet environment to avoid confusion with the
existing file (which may potentially have a different payload).

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
refactor: change user execution error to abc
8f25316 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
chore: update date in copyright notice line
498e23c 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
chore: remove todo, feature was implemented
eb4aafc 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
fix: force select selector
083db48 
Using DefaultSelector fails in jammy with Python 3.10. It seems that
this specific version picks a different selector, or there's some
problem with SelectSelector that prevents it from working as expected.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka force-pushed the CRAFT-3828-Capture-error-in-step-scriptlet-execution branch from 389e90f to 8362616 Compare January 17, 2025 16:47
@cmatsuoka
chore: revert unnecessary ruff warning change
ffa6125 
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka force-pushed the CRAFT-3828-Capture-error-in-step-scriptlet-execution branch from 8362616 to ffa6125 Compare January 17, 2025 17:24
lengau
lengau approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Brilliant!

I think a correct conventional-commits title for this would start with "feat(executor):"

@sergiusens sergiusens merged commit 49cde07 into main Jan 18, 2025
13 checks passed
@sergiusens sergiusens deleted the CRAFT-3828-Capture-error-in-step-scriptlet-execution branch January 18, 2025 00:16
@cmatsuoka cmatsuoka mentioned this pull request Jan 24, 2025
Closed
tigarmo added a commit that referenced this pull request Feb 3, 2025
@clay-lake @lengau
@sergiusens @renovate @tigarmo @bepri @mr-cal @carlcsaposs-canonical @yanksyoon @cmatsuoka @dariuszd21 @sergio-costas @jahn-junior @vpa1977 @medubelko
feat: updates to complete feature set for RK042 - Support for Pro Roc…
0166434 
…ks in Rockcraft. (#993)

* style: autoformat

* style: fix spelling

* build(deps): fix dependencies to make tests succeed

* style: adjust pylint configuration to make lint pass

* ci: install chisel v0.10.0

* fix: chisel tests on 20.04

* style: autoformat

* style: fix spelling

* build(deps): fix dependencies to make tests succeed

* style: adjust pylint configuration to make lint pass

* ci: install chisel v0.10.0

* fix: chisel tests on 20.04

* fix: pyright

* fix(ci): 20.04 packages

* tests: create integration tests for talking to snapd

* build(deps): replace requests-unixsocket with requests-unixsocket2

Fixes #852

* fix: bump python version

* chore: bump version to 1.19.8

* tests: create integration tests for talking to snapd

* build(deps): replace requests-unixsocket with requests-unixsocket2

* fix: bump python version

* Bump version: 1.25.2 → 1.25.3

* style: fix linting errors

* feat: add an API for registering source types (#927)

This provides a pair of functions (sources.register, sources.unregister)
for registering and unregistering source types. This differs slightly
from plugin registration in current sources cannot be removed.

* build: migrate to pyproject.toml (#902)

Removes dependency on bumpversion in favor of git tags for versioning.

Co-authored-by: Sergio Schvezov <sergio.schvezov@canonical.com>

* build(deps): update dependency myst-parser to v4 (#871)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat: use "craft.git" for git where possible (#925)

If "craft.git" is available on the PATH, use it. Otherwise, fallback to "git".

Fixes #900

* doc: create new how-to on creating a plugin (#928)

* feat: add uv plugin (#930)

* fix: remove captured message on error (#937)

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

* docs: move changelog, add PR task (#942)

* docs: move changelog to reference

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

* ci: add PR task for changelog updates

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

---------

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

* feat(plugins/poetry): add poetry and pip extra args (#940)

This allows more customisation of the poetry and pip commands for advanced users.

* docs(changelog): add 2.2.0 changes (#941)

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

* doc: update docs with extra ref anchors (#945)

* tests: deduplicate tests for python-based plugins (#947)

* docs: clarify poetry-with field (#946)

Co-authored-by: Alex Lowe <alex.lowe@canonical.com>

* chore: fix some small pyproject errors, include uv.lock (#955)

The pyproject file referenced some deprecated Ruff linter rules and incorrectly formatted the git describe command parameter, which had to be an array. This commit fixes both

* fix: make uv plugin re-entrant on code changes (#956)

* fix: correct git versioning error (#957)

* build(deps): update dependency mypy to ~=1.14.1 (#958)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(docs): include poetry links as a target (#960)

This allows applications that have their own custom poetry reference to
include all the default links by adding the following to the bottom
of their poetry plugin reference:

.. include:: /common/craft-parts/reference/plugins/poetry_plugin.rst
    :start-after: .. _craft_parts_poetry_links:

Signed-off-by: Alex Lowe <alex.lowe@canonical.com>

* style: fix linting issues for ruff 0.9 (#969)

* build(deps): update bugfixes (#952)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* ci: replace Renovate config with starflow (#968)

Co-authored-by: Callahan <callahan.kovacs@canonical.com>

* docs: update docstring for maven plugin (#971)

Co-authored-by: Callahan <callahan.kovacs@canonical.com>
Co-authored-by: Alex Lowe <alex.lowe@canonical.com>

* feat(executor): allow capturing error message from scriptlets (#966)

* change craftctl channel to socket

Change to craftctl communication channel from named pipes to unix
sockets to make it easier to consolidate the ctl server and error
stream capture.

* allow using process selector with custom handlers

Process execution runs a selector to obtain data from i/o streams
to obtain the isolated error output. Allow this selection loop run
on a caller-specified selector with handlers registered to also
process interactions with craftctl.

* change scriptlet environment script name

Step execution environment is step-specific, and the existing
environment.sh script is for the build step only. Rename the file
containing the scriptlet environment to avoid confusion with the
existing file (which may potentially have a different payload).

* force select selector

Using DefaultSelector fails in jammy with Python 3.10. It seems that
this specific version picks a different selector, or there's some
problem with SelectSelector that prevents it from working as expected.

---------

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

* docs: update changelog (#984)

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>

* fix: use source-subdir in go-use plugin

It's possible, and somewhat common, for a go module to be defined inside a
repo's subdir; in this case we can define source-subdir, but the go-use
plugin needs to use this - otherwise the repo root gets used.

Fixes #978

* docs: fix changelog, add go-use bugfix

* feat(PartSpec): add source-channel attribute (#981)


Signed-off-by: Dariusz Duda <dariusz.duda@canonical.com>

* docs(changelog): finalize changelog for 2.4.0 (#988)

Signed-off-by: Dariusz Duda <dariusz.duda@canonical.com>

* fix: don't modify 'prefix' for relocatable pkg-config files (#977)

A relocatable `.pc` file has a `prefix` key beginning with
`${pcfiledir}`. The `prefix` key is not modified if the `.pc`
file is relocatable.

* fix(tests): don't fail if you have additional JREs (#986)

Fixes #985

* docs(changelog): finalize changelog for 2.4.1 (#990)

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

* feat: craftctl chroot command

* docs: update and complete the step execution environment (#949)

* docs: update and complete the step execution environment

This adds a large number of missing environment variables from the step
execution environment and updates the information about CRAFT_TARGET_ARCH
and CRAFT_ARCH_TRIPLET being deprecated.

Co-authored-by: Callahan <callahan.kovacs@canonical.com>
Co-authored-by: JJ Coldiron <jj.coldiron@canonical.com>

* fix(craft_parts/executor/step_handler.py): adjust subprocess command to stream output

* fix:adjustments for linter

* fix(tests/integration/lifecycle/test_craftctl.py): update expected error description

* feat: add Jlink plugin (#951)

Co-authored-by: Tiago Nobrega <tiago.nobrega@canonical.com>
Co-authored-by: Michael DuBelko <michael.dubelko@gmail.com>
Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

* docs: correct the lifecycle step for `organize` (#996)

Co-authored-by: Callahan <callahan.kovacs@canonical.com>

* fix(craft_parts/utils/process.py): migrate subprocess.run to internal process.run

---------

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Alex Lowe <alex.lowe@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Dariusz Duda <dariusz.duda@canonical.com>
Co-authored-by: Alex Lowe <alex.lowe@canonical.com>
Co-authored-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Tiago Nobrega <tiago.nobrega@canonical.com>
Co-authored-by: Imani Pelton <imani.pelton@canonical.com>
Co-authored-by: Callahan <callahan.kovacs@canonical.com>
Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>
Co-authored-by: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com>
Co-authored-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Co-authored-by: Dariusz Duda <dariusz.duda@canonical.com>
Co-authored-by: Sergio Costas <sergio.costas@canonical.com>
Co-authored-by: JJ Coldiron <jj.coldiron@canonical.com>
Co-authored-by: Vladimir Petko <vladimir.petko@canonical.com>
Co-authored-by: Michael DuBelko <michael.dubelko@gmail.com>
Co-authored-by: Michael DuBelko <michael.dubelko@canonical.com>
@bepri bepri mentioned this pull request Feb 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergiusens sergiusens sergiusens left review comments

@tigarmo tigarmo tigarmo approved these changes

@lengau lengau lengau approved these changes

@bepri bepri bepri approved these changes

@mr-cal mr-cal Awaiting requested review from mr-cal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cmatsuoka @sergiusens @tigarmo @lengau @bepri