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

Copy dpkg.yaml for LP Buildd #125

Merged
merged 2 commits into from Sep 28, 2021
Merged

Conversation

ilasc
Copy link
Contributor

@ilasc ilasc commented Sep 16, 2021

We will need to copy the yaml file in order to be able to gather it from the Buildd output folder.

Makefile Outdated
if [ -e /build/core ]; then \
$(SUDO) mv livecd.ubuntu-core.manifest /build/core/core_16-$$(cat $(DESTDIR)/usr/lib/snapd/info|cut -f2 -d=|cut -f1 -d~|cut -b1-29)_$(DPKG_ARCH).manifest; \
$(SUDO) cp usr/share/snappy/dpkg.yaml /build/core/core_16-$$(cat $(DESTDIR)/usr/lib/snapd/info|cut -f2 -d=|cut -f1 -d~|cut -b1-29)_$(DPKG_ARCH).dpkg.yaml; \
Copy link

Choose a reason for hiding this comment

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

It would be nice to create a variable containing all those cut pipes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mardy Thanks for the review, I added a variable for thecut pipes and tested under LP Buildd itself that the snap builds successfully and produces the expected files (copied in the right places) with the correct version: https://launchpad.net/~ilasc/+snap/core-recipe/+build/1532608 and buildlog for details: https://launchpadlibrarian.net/559807750/buildlog_snap_ubuntu_xenial_amd64_core-recipe_BUILDING.txt.gz

Copy link

Choose a reason for hiding this comment

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

I actually meant something like this:

TARGET_BASENAME=/build/core/core_16-$$(cat $(DESTDIR)/usr/lib/snapd/info|cut -f2 -d=|cut -f1 -d~|cut -b1-29)_$(DPKG_ARCH) \
$(SUDO) mv livecd.ubuntu-core.manifest $(TARGET_BASENAME).manifest; \
$(SUDO) cp /build/core/parts/livebuild/install/usr/share/snappy/dpkg.yaml $(TARGET_BASENAME).dpkg.yaml; \

(untested)

Sorry for the confusion! :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mardy Thanks for the suggestion Alberto! I tried something similar yesterday and various forms of it, it looks like any variable that contains the info file will result in failure (from what I could work out is evaluates before the file exists) - the build will be missing the needed dpkg and manifest files: https://launchpad.net/~ilasc/+snap/core-recipe/+build/1532705,  build log:  https://launchpadlibrarian.net/559822471/buildlog_snap_ubuntu_xenial_amd64_core-recipe_BUILDING.txt.gz
The only way I found to produce a successful build with the necessary files and correct suffix is the using a variable that doesn't attempt to contract that command any further than the cut pipes. 

Copy link

Choose a reason for hiding this comment

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

Ah, I think that we have to play a bit with make in order to properly expand the variable. This should work:

TARGET_BASENAME=/build/core/core_16-$$(cat $(DESTDIR)/usr/lib/snapd/info|cut -f2 -d=|cut -f1 -d~|cut -b1-29)_$(DPKG_ARCH) \
$(SUDO) mv livecd.ubuntu-core.manifest $${TARGET_BASENAME}.manifest; \
$(SUDO) cp /build/core/parts/livebuild/install/usr/share/snappy/dpkg.yaml $${TARGET_BASENAME}.dpkg.yaml; \

Copy link

Choose a reason for hiding this comment

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

Mmm... the last one is the most encouraging! Last attempt: ${TARGETBASENAME} :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mardy I had already tried that one last week already unfortunately with the same results, but I've put it through Buildd again this morning to be able to share with you: Commit: https://git.launchpad.net/~ilasc/+git/core/commit/?id=da3eaf57d0c4b9b343e08192bc89735098809ced, Build without manifest and dpkg files: https://launchpad.net/~ilasc/+snap/core-recipe/+build/1536824 and Buildlog: https://launchpadlibrarian.net/560367296/buildlog_snap_ubuntu_xenial_amd64_core-recipe_BUILDING.txt.gz

Copy link
Collaborator

@mvo5 mvo5 Sep 28, 2021

Choose a reason for hiding this comment

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

Sorry for joining so late - I looked at your build number (2): $$TARGETBASENAME (which really ought to work) and there is a successful buildlog and also a resulting snap that has dpkg.yaml - is the buildlog misleading me?

Sorry again, I did not pay close enough attention - can you please try to add a ; after TARGETBASENAME=... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mvo5 Hi Michael,

For us to call a core snap build successful the build needs to produce this output: a .snap file, a .manifest file and a dpkg.yaml file, example of a successful build produced by the code proposed in this PR: https://launchpad.net/~ilasc/+snap/core-recipe/+build/1532608

Build build number (2): $$TARGETBASENAME is not a successful build as it is missing both the manifest file and dpkg.yaml file.

What is proposed in this PR is producing a successful build (https://launchpad.net/~ilasc/+snap/core-recipe/+build/1532608) and I maintain the statement in my early comment on the PR: it is pretty clear we cannot contract that path variable further than the code in this PR does as any variable that contains the info file will result in failure (it evaluates before the file exists) due to the way the snap build works under Buildd (as can be seen from the list of attempts made here with these builds: https://launchpad.net/~ilasc/+snap/core-recipe).

The goal of this PR is to copy alongside the manifest file the new dpkg.yaml which is does successfully. The discussion spawned here is around refactoring existing code that was already working successfully for moving the manifest file to make it more readable. I agree it's okay to move forward without refactoring any further the way we build that path for the files to be copied / moved to. 

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's land it and do a followup then.

@mvo5 mvo5 merged commit 403d3f0 into snapcore:master Sep 28, 2021
mvo5 added a commit to mvo5/core that referenced this pull request Sep 28, 2021
mvo5 added a commit to mvo5/core that referenced this pull request Sep 28, 2021
mvo5 added a commit to mvo5/core that referenced this pull request Sep 28, 2021
mvo5 added a commit to mvo5/core that referenced this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants