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

Install pebble via implicit part #59

Merged
merged 11 commits into from
Sep 30, 2022
Merged

Conversation

cjdcordeiro
Copy link
Collaborator

Since Pebble will be the entrypoint for all ROCKs, we must have it installed for all ROCKs too.

Similar to what Kerncraft is doing at https://github.com/canonical/kerncraft/pull/22/files#diff-a5aa2c8752af7f6a4943ee8761dd913a6a7662e7d5e7b4c80602e225070617d2, also in Rockcraft we must add a part for building and installing Pebble into the ROCK.

The difference between this PR and the Kerncraft implementation is that 1) the Pebble part is implicit (always there), and 2) there's no need for a custom Pebble plugin, since Pebble is just a Go application, so we can just use the already existing Go plugin from craft-cli.

  • Have you signed the CLA?

@cjdcordeiro
Copy link
Collaborator Author

@sergiusens @cmatsuoka @tigarmo pls note the comment in https://github.com/canonical/rockcraft/pull/59/files#diff-0ea7d1a44d76c7792a852505490ca9e3fb738ec2a7814b051ebc828efb8ceceeR108 (it isn't a serious problem since we have unit tests which would flag an invalid part spec, but still, maybe there is a better place for this)

rockcraft/project.py Outdated Show resolved Hide resolved
rockcraft/project.py Outdated Show resolved Hide resolved
@cjdcordeiro
Copy link
Collaborator Author

the CI error could be an intermittent one. could you please re-run the workflow?

@cjdcordeiro
Copy link
Collaborator Author

I'm not sure what the error is about. I can't reproduce it locally. I've run spread and also directly rockcraft, and couldn't reproduce.

@tigarmo
Copy link
Collaborator

tigarmo commented Sep 12, 2022

Weird, it failed again:

subprocess.CalledProcessError: Command '['snap', 'install', 'go', '--channel', '1.19/stable', '--classic']' returned non-zero exit status 1.

@tigarmo
Copy link
Collaborator

tigarmo commented Sep 12, 2022

I think the other tests use an older version of go; maybe we should use latest/stable?

@cjdcordeiro
Copy link
Collaborator Author

oh but now the error was different. So the go installation failed due to lack of disk space

@tigarmo
Copy link
Collaborator

tigarmo commented Sep 12, 2022

Very curious, these machines have 40G of storage I think. @sergiusens any insight into why this would fail?

@tigarmo tigarmo marked this pull request as draft September 28, 2022 17:13
The default build commands of the 'go' plugin call 'go generate ./...',
but that doesn't work for pebble (or chisel). Until that default
behavior can be disabled/customized we need to override the entire step.
This provides a way to "customize" pebble inside the ROCKs: if a
project's spec already contains a "pebble" part, we preserve it and
assume it's working. This is done in load_project(), _before_
unmarshaling, to be consistent with what Snapcraft does for similar
"special" parts.

The new _add_pebble_data() will also be useful in the future when we
have to configure entrypoints, pebble env variables, etc.
@tigarmo tigarmo marked this pull request as ready for review September 29, 2022 17:23
@tigarmo
Copy link
Collaborator

tigarmo commented Sep 29, 2022

Hi @cjdcordeiro, I updated the PR with a few changes:

Please take a look at the changes, and if you +1 them I'll go ahead and approve/merge the PR. Thanks!

@cjdcordeiro
Copy link
Collaborator Author

Thanks @tigarmo ! Nice work.

It all lgtm except https://github.com/canonical/rockcraft/pull/59/files#diff-0ea7d1a44d76c7792a852505490ca9e3fb738ec2a7814b051ebc828efb8ceceeR493. This is not according to our spec, because it allows users to create a ROCK without Pebble as the entrypoint.

I understand that the objective is to align with #65, but we don't need to do this now. My fear is that we are introducing a capability which later on must be removed ("being able of not having pebble").

@tigarmo
Copy link
Collaborator

tigarmo commented Sep 30, 2022

This is not according to our spec, because it allows users to create a ROCK without Pebble as the entrypoint.

How would the users create a ROCK without Pebble as the entrypoint? Do you mean if they have a part called pebble that actually has unrelated things?

@cjdcordeiro
Copy link
Collaborator Author

This is not according to our spec, because it allows users to create a ROCK without Pebble as the entrypoint.

How would the users create a ROCK without Pebble as the entrypoint? Do you mean if they have a part called pebble that actually has unrelated things?

exactly. something like

parts:
  pebble:
    plugin: nil

Which could be there either by mistake or by an ill-intended desire to avoid Pebble

@tigarmo
Copy link
Collaborator

tigarmo commented Sep 30, 2022

I see. I updated the code to fail if a pebble part is already present, and we can revisit the customization support in the near future

@cjdcordeiro
Copy link
Collaborator Author

perfect

@tigarmo tigarmo enabled auto-merge (squash) September 30, 2022 15:14
@tigarmo tigarmo merged commit 5d7ed67 into canonical:main Sep 30, 2022
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.

None yet

3 participants