search

canonical / core18

The core18 base snap
14 stars 26 forks source link

Make the version number date-based. #121

Closed sil2100 closed 5 years ago

sil2100 commented 5 years ago

Pushing this as a PR since for unknown reasons the version-script fails here locally. Based on mvo's earlier PR.

sil2100 commented 5 years ago

@mvo5 maybe you know: CI seems to be failing for this branch, and when building it locally it also fails for me (cosmic) - does this work when you build it locally? I originally thought there's something wrong with my system only, as I didn't see anything wrong with the change I proposed.

sil2100 commented 5 years ago

It's hard for me to understand how we are actually tainting the snapcraft environment that the version script doesn't work (it works fine on any other snapcraft project). Should we pull in someone from the snapcraft team to help?

mvo5 commented 5 years ago

@sil2100 I poked at this a bit last night but had some network issues (was on a slow link for unrelated reasons). I poke at it again today, I suspect its snapcraft.internal.common - it has a "env = []" list that is probably not cleaned.

mvo5 commented 5 years ago

I filed https://bugs.launchpad.net/snapcraft/+bug/1822988 - that hopefully explains why it crashes. I don't know any workarounds right now.

sergiusens commented 5 years ago

Two options here, try and use the replacement for the deprecated version-script which is documented here https://docs.snapcraft.io/scriptlets/4892 and do it early enough in the process, like

Extract:

# Comment version as it takes precedence
# version: foo
adopt-info: core

parts:
  core:
    ...
    override-pull: |
      snapcraftctl set-version $(/bin/date ...)
      snapcraftctl pull

Or to run bin/date with LD_LIBRARY_PATH set to "", such that version script looks like

version-script: echo "$(LD_LIBRARY_PATH="" /bin/date +%Y%M%d)"
sil2100 commented 5 years ago

Wow, that's something. Thanks Sergio, Michael! Didn't know version-script was being deprecated, geh. I actually tried the version-script: echo "$(LD_LIBRARY_PATH="" /bin/date +%Y%M%d)" and it didn't work, so I'm glad there's something else like this. All good to go, let me merge this!

Thanks mvo for force-pushing!

mvo5 commented 5 years ago

@sergiusens Thanks a bunch for your suggestions! it does no longer crash with set-version. But apparently it also does not work :/ Any ideas.

sergiusens commented 5 years ago

Yes, the adopt-info part is key into making this be picked up.

sergiusens commented 5 years ago

El miércoles, 3 de abril de 2019 13:57:14 -03 Łukasz Zemczak escribió:

Wow, that's something. Thanks Sergio, Michael! Didn't know version-script was being deprecated, geh. I actually tried the version-script: echo "$(LD_LIBRARY_PATH="" /bin/date +%Y%M%d)" and it didn't work, so I'm glad there's something else like this. All good to go, let me merge this!

This is because I might have gotten it wrong and the env var needs to be set even for echo. Thinking a bit more, you do not even need the echo, all you need is to output to stdout which calling date already does! :-)

sergiusens commented 5 years ago

Seems to have done the trick Snapped core18_20194603_amd64.snap

sil2100 commented 5 years ago

Thanks again Sergio! Yes, it seems to work like a charm now ;)