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

snap: v0: build fixes & updates #3999

Merged
merged 18 commits into from
Jun 13, 2020
Merged

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Jun 9, 2020

FAILED tests/func/test_repro.py::TestReproExternalSSH::test - dvc.exceptions....
FAILED tests/func/test_repro_multistage.py::TestReproExternalSSHMultiStage::test

@casperdcl casperdcl added bug Did we break something? p0-critical labels Jun 9, 2020
@casperdcl casperdcl requested a review from efiop June 9, 2020 21:03
@casperdcl casperdcl self-assigned this Jun 9, 2020
@casperdcl casperdcl mentioned this pull request Jun 9, 2020
8 tasks
@casperdcl casperdcl added p1-important Important, aka current backlog of things to do and removed p0-critical labels Jun 9, 2020
Comment on lines 54 to 68
# fetch tags for `git-describe`, since
# - can't rely on $TRAVIS_TAG for `edge` (master) releases, and
# - `snapcraft` also uses `git-describe` for version detection
git fetch --tags
TAG_MAJOR="$(git describe --tags | sed -r 's/^v?([0-9]+)\.[0-9]+\.[0-9]+.*/\1/')"
[[ -n "$TAG_MAJOR" ]] || exit 1 # failed to detect major version
if [[ -n "$TRAVIS_TAG" ]]; then
if [[ $(echo "$TRAVIS_TAG" | grep -E '^[0-9]+\.[0-9]+\.[0-9]+$') ]]; then
echo "export SNAP_CHANNEL=stable" >>env.sh
echo "export SNAP_CHANNEL=stable,v$TAG_MAJOR/stable" >>env.sh
else
echo "export SNAP_CHANNEL=beta" >>env.sh
echo "export SNAP_CHANNEL=beta,v$TAG_MAJOR/beta" >>env.sh
fi
else
echo "export SNAP_CHANNEL=edge" >>env.sh
echo "export SNAP_CHANNEL=edge,v$TAG_MAJOR/edge" >>env.sh
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, missed that this is only for 0.94-dev branch. I guess we need to port to the master too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will do later

@efiop
Copy link
Contributor

efiop commented Jun 12, 2020

@casperdcl What's the status on this PR?

@casperdcl
Copy link
Contributor Author

casperdcl commented Jun 12, 2020

I've thought long and hard about this - the only reservation I have is that the hardcoded warning will display even if users run refresh --channel=v0/stable. Ideally the warning should be suppressed in that case, but doing that would require either

  • having a different branch/tag pushing to v0/stable without the hardcoded warning, or
  • building the snap twice - before and after doing git apply remove_warning.patch

"{red}WARNING{reset}: ignoring this message will result in\n"
"snap automatically performing an upgrade soon.\n"
"More information can be found at\n"
"{blue}https://github.com/iterative/dvc/issues/3872{reset}"
Copy link
Contributor Author

@casperdcl casperdcl Jun 12, 2020

Choose a reason for hiding this comment

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

Ideally need a dvc.org link to migration/breaking changes.

https://dvc.org/doc/install/pre-release is currently not quite appropriate.

@efiop
Copy link
Contributor

efiop commented Jun 12, 2020

@casperdcl Maybe let's not bother with the warning at all? Running snap from dvc on each command is a definite no-go.

original_snap_name="$(ls dvc_*.snap)"
mv "$original_snap_name" original.snap

git apply scripts/remove_update_warning.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make dvc --version show up as dirty

Copy link
Contributor Author

@casperdcl casperdcl Jun 12, 2020

Choose a reason for hiding this comment

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

yes; I don't think that's an issue.

@casperdcl
Copy link
Contributor Author

yes nothing's run each command - it's all hard-coded now for snap releases:

  • no warning for v0 channel
  • warn for 0.x on latest channel

@casperdcl
Copy link
Contributor Author

@efiop this should be fine to merge now. The two unchecked boxes are optional follow-up items, really.

@efiop efiop merged commit 6f0a05a into iterative:0.94.0-dev Jun 13, 2020
@casperdcl casperdcl mentioned this pull request Jun 13, 2020
casperdcl added a commit to casperdcl/dvc that referenced this pull request Jun 13, 2020
efiop pushed a commit that referenced this pull request Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? p1-important Important, aka current backlog of things to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants