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

Auto update app image #3736

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Auto update app image #3736

merged 1 commit into from
Oct 20, 2021

Conversation

Thatoo
Copy link
Contributor

@Thatoo Thatoo commented Sep 2, 2021

I guess it's a way to solve this two issues :
#168
#2656

@mgallien
Copy link
Collaborator

mgallien commented Sep 2, 2021

@Thatoo thanks for your contribution
that is interesting especially since I did not knew Appimage could provide a kind of automated update feature
can you have a look at the failed DCO CI check and fix it ?
we will do a proper review of the PR as soon as we can

@antony-jr
Copy link

antony-jr commented Sep 2, 2021

EDIT: Just a heads up, make sure you install zsync in your build system to enable linuxdeployqt to generate .zsync file automatically (ubuntu has it: apt install zsync).

@Thatoo LGTM 👍

I've tested your PR AppImage with update tool and I can see that the update information is parsed correctly. It fails for now since the .zsync file is not in the assets of the latest release but should work fine once the .zsync file is uploaded to the releases on next release hopefully.

   INFO:   setAppImage :  "/home/antonyjr/Downloads/Nextcloud-PR-3736-cba992a809d8ec4623f87ccaa31d45b1498beab3-x86_64.AppImage" . 
   INFO:   getInfo : AppImage is confirmed to be type 2. 
   INFO:   getInfo : mapping AppImage to memory. 
   INFO:   getInfo : AppImage architecture is x86_64 (64 bits). 
   INFO:   getInfo : updateString( "gh-releases-zsync|nextcloud|desktop|latest|Nextcloud-*-x86_64.AppImage.zsync" ). 
   INFO:   getInfo : finished. 
   INFO:  setControlFileUrl : using github releases zsync transport. 
   INFO:  setControlFileUrl : github api request( QUrl("https://api.github.com/repos/nextcloud/desktop/releases/latest") ). 
   INFO:   handleGithubAPIResponse : starting to parse github api response. 
   INFO:   handleGithubAPIResponse : http response code( 200 ). 
   INFO:  handleGithubAPIResponse : latest version is  "v3.3.2" 
   INFO:  handleGithubAPIResponse : asset required is  "Nextcloud-*-x86_64.AppImage.zsync"

@FlexW
Copy link

FlexW commented Sep 3, 2021

We first need to merge #3682. Otherwise, we have to duplicate everything in Brander.

@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from cba992a to 43d8b33 Compare September 4, 2021 01:37
@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 4, 2021

@Thatoo thanks for your contribution
that is interesting especially since I did not knew Appimage could provide a kind of automated update feature
can you have a look at the failed DCO CI check and fix it ?
we will do a proper review of the PR as soon as we can

@mgallien sorry for late reply but DCO is solved now.

@mgallien
Copy link
Collaborator

mgallien commented Oct 6, 2021

@mgallien sorry for late reply but DCO is solved now.

that is my turn to apologize

I will make sure to quickly review your PR

sorry

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

I am still missing one step with this
how does the update will happen
from my understanding we would also need to provide an update tool within the appimage and plug it to the UI button

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 7, 2021

Maybe @probonopd or @TheAssassin could answer your question and help us improve Nextcloud's AppImage.

@antony-jr
Copy link

I am still missing one step with this how does the update will happen from my understanding we would also need to provide an update tool within the appimage and plug it to the UI button

The first step is including the update information then other external AppImage tools (such as the official https://github.com/AppImage/AppImageUpdate) can delta update any nextcloud AppImages. Even AppImageLauncher can do that.

But if you want your own AppImage to do this delta update then you can use the official C++ library used in the official update tool -> https://github.com/AppImage/AppImageUpdate

Side Note: I built an unofficial AppImage update library tailored for Qt developers, see the docs to get some idea on how this works here

@mgallien
Copy link
Collaborator

mgallien commented Oct 7, 2021

I am still missing one step with this how does the update will happen from my understanding we would also need to provide an update tool within the appimage and plug it to the UI button

The first step is including the update information then other external AppImage tools (such as the official https://github.com/AppImage/AppImageUpdate) can delta update any nextcloud AppImages. Even AppImageLauncher can do that.

But if you want your own AppImage to do this delta update then you can use the official C++ library used in the official update tool -> https://github.com/AppImage/AppImageUpdate

Side Note: I built an unofficial AppImage update library tailored for Qt developers, see the docs to get some idea on how this works here

thanks

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

see my comment about the org name

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 11, 2021

you mean like
./squashfs-root/AppRun ${DESKTOP_FILE} -appimage -updateinformation="gh-releases-zsync|nextcloud-releases|desktop|latest|Nextcloud-*-x86_64.AppImage.zsync"
instead of
./squashfs-root/AppRun ${DESKTOP_FILE} -appimage -updateinformation="gh-releases-zsync|nextcloud|desktop|latest|Nextcloud-*-x86_64.AppImage.zsync"

@antony-jr
Copy link

antony-jr commented Oct 11, 2021

you mean like ./squashfs-root/AppRun ${DESKTOP_FILE} -appimage -updateinformation="gh-releases-zsync|nextcloud-releases|desktop|latest|Nextcloud-*-x86_64.AppImage.zsync" instead of ./squashfs-root/AppRun ${DESKTOP_FILE} -appimage -updateinformation="gh-releases-zsync|nextcloud|desktop|latest|Nextcloud-*-x86_64.AppImage.zsync"

Yes. I suppose. Since the nextcloud part denotes the organization or github username. The third part desktop denotes the repo under the org or github username.

Please See: https://github.com/AppImage/AppImageSpec/blob/master/draft.md#github-releases

@Thatoo Thatoo requested a review from mgallien October 11, 2021 15:27
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 11, 2021

DCO is not happy again. I don't know what is it and why it is not happy. I did all contribution through web browser being logged in...
Should I start over?

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

thanks

@allexzander
Copy link
Contributor

allexzander commented Oct 12, 2021

DCO is not happy again. I don't know what is it and why it is not happy. I did all contribution through web browser being logged in... Should I start over?

@Thatoo Your other comments have this:

Signed-off-by: Thatoo [email protected]

in their description. Please update your last commit so it follows the same pattern as the previous ones. Also, for the future, please do not use "merge master" to your branch, but, use "rebase against a master" instead. I.E. do not click the "Update branch" button.

@FlexW
Copy link

FlexW commented Oct 13, 2021

@mgallien @allexzander @Thatoo as far as I understand we need to have zsync installed for this patch to work right? I've created a pr here nextcloud/docker-ci#319

@mgallien
Copy link
Collaborator

@Thatoo can you rebase ?

@mgallien
Copy link
Collaborator

@mgallien @allexzander @Thatoo as far as I understand we need to have zsync installed for this patch to work right? I've created a pr here nextcloud/docker-ci#319

Thanks for noticing that I had forgotten this

@FlexW
Copy link

FlexW commented Oct 14, 2021

@mgallien @Thatoo I guess it would make sense before rebasing if the pull request uses the newest docker image for the CI (the one with zsync).

@Thatoo can you please change that line

image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-2
to image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-3

@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from 018e7b2 to 990a63a Compare October 14, 2021 08:33
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 14, 2021

Is it better now?

@FlexW
Copy link

FlexW commented Oct 14, 2021

@Thatoo You don't need any special token, but every commit needs to have the line
Signed-off-by: Thatoo [email protected] in it.

You also need to squash your commits into one commit and then rebase. You can't do that with the webui.
I'm also not sure what you did because you now have a lot of unnecessary commits in your pull request.

https://git-scm.com/docs/git-rebase

If that is too much for you, we can do it for you, but you will probably lose ownership over the changes.

@FlexW
Copy link

FlexW commented Oct 19, 2021

@Thatoo looks better now :) Still, it seems like you did not rebase properly. Make sure your local master branch of your fork is up to date. For that make sure you have the upstream configured correctly as I told you above :) and then checkout master (git checkout master) and execute git pull upstream master and then rebase your feature branch with git checkout AutoUpdateAppImage and git rebase master. Then upload the changes with git push --force origin AutoUpdateAppImage

@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from 9e9c700 to 95ca683 Compare October 19, 2021 09:49
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 19, 2021

I guess I did it, DCO is happy but it couldn't build it...

@FlexW
Copy link

FlexW commented Oct 20, 2021

/rebase

@FlexW
Copy link

FlexW commented Oct 20, 2021

@Thatoo I'm unsure what the problem is (Maybe too many requests). Could you enable the setting "Allow edits from maintainers" https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork then we can rebase it through the webui which will be easier for us. Just be sure if you need to again make changes that you need to pull the changes first with git pull --rebase (the --rebase is very important. Otherwise, you will have unwished merge commits). Could you also update the pr to use version 4 of the docker image (there was an unforeseen update yesterday)? Sorry for all the back and forth.

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 20, 2021

I guess you could already edit as the option was already enabled :
image

@FlexW
Copy link

FlexW commented Oct 20, 2021

Oh okay, then the rebase is not possible for forks. Maybe if you do the change that I suggested and rebase, the CI will also run through and we can finally merge it.

@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from 95ca683 to bd5017a Compare October 20, 2021 10:17
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 20, 2021

git pull --rebase upstream master
# change 3 to 4
git add .
git commit -s -m "docker image 4"
git push --force origin AutoUpdateAppImage

Is it ok?

@FlexW
Copy link

FlexW commented Oct 20, 2021

@Thatoo Almost ;) You need to first update your master branch with:
git checkout master
git pull upstream master
Then you can checkout again your feature branch
git checkout AutoUpdateAppImage
Then rebase your branch
git rebase master
And then you need to perform an interactive rebase to squash your two commits into one because the second commit is not of value on it's own. Do this with:
git rebase -i HEAD~2
In the editor that opens change for the second commit pick to squash and then exit the editor. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

For the future if you do some small change and you know that should be part of the commit before it, instead of creating a new commit amend the changes to the old commit. That will look like this:
git add .
git commit --amend --no-edit.

Last but not least upload your changes:
git push --force

Btw that we are sure your upstream is configured correct. Does git remote -v gives similar output to this:

origin	[email protected]:FlexW/desktop.git (fetch)
origin	[email protected]:FlexW/desktop.git (push)
upstream	[email protected]:nextcloud/desktop.git (fetch)
upstream	[email protected]:nextcloud/desktop.git (push)

If it's https instead of git it doesn't matter.

@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from bd5017a to 9662f71 Compare October 20, 2021 10:39
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 20, 2021

$ git remote -v
origin	https://github.com/Thatoo/desktop.git (fetch)
origin	https://github.com/Thatoo/desktop.git (push)
upstream	https://github.com/nextcloud/desktop.git (fetch)
upstream	https://github.com/nextcloud/desktop.git (push)

.drone.yml Outdated
@@ -98,7 +98,7 @@ name: AppImage

steps:
- name: build
image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-2
image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-4
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be image: ghcr.io/nextcloud/continuous-integration-client-appimage:client-appimage-3

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 20, 2021

@Thatoo I'm unsure what the problem is (Maybe too many requests). Could you enable the setting "Allow edits from maintainers" https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork then we can rebase it through the webui which will be easier for us. Just be sure if you need to again make changes that you need to pull the changes first with git pull --rebase (the --rebase is very important. Otherwise, you will have unwished merge commits). Could you also update the pr to use version 4 of the docker image (there was an unforeseen update yesterday)? Sorry for all the back and forth.

So I guess I misunderstood this. What is this version 4 of the docker image? What should I change about that?

@FlexW
Copy link

FlexW commented Oct 20, 2021

@mgallien That was my mistake because I thought we pushed out a new version. Then I checked and in fact, not even version 3 was pushed out, so I have built it an hour ago. @Thatoo You did not misunderstand something. I told you to do the wrong thing. Replace 4 with 3 and everything should be good. Sorry. And please rebase again and then we try to merge that as quickly as possible before anything else gets merged again ;)

@mgallien
Copy link
Collaborator

@mgallien That was my mistake because I thought we pushed out a new version. Then I checked and in fact, not even version 3 was pushed out, so I have built it an hour ago. @Thatoo You did not misunderstand something. I told you to do the wrong thing. Replace 4 with 3 and everything should be good. Sorry. And please rebase again and then we try to merge that as quickly as possible before anything else gets merged again ;)

OK I had missed the 4 in of your previous comment

Signed-off-by: Thatoo <[email protected]>

docker image 4

Signed-off-by: Thatoo <[email protected]>
@Thatoo Thatoo force-pushed the AutoUpdateAppImage branch from 9662f71 to 3ab698a Compare October 20, 2021 13:51
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-3736-3ab698a0c59291310300f2124983634200e0ae2e-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@FlexW FlexW merged commit ed7abc4 into nextcloud:master Oct 20, 2021
@FlexW
Copy link

FlexW commented Oct 20, 2021

Thanks @Thatoo :)

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 21, 2021

@TheAssassin : Why does it seem broken? and what could be the fix?

@TheAssassin
Copy link

You obviously try to assign a variable, but use two =. That, I suppose, does not work.

Aside from that, I'd strongly recommend you to run shellcheck on the entire file. There are tons of escaping issues.

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 21, 2021

Indeed, even when I asked the question, I was looking at the line you highlighted and I didn't notice this second = ...
Thanks.
The problem is that I'm afraid to make a mistake, I don't know how I should commit a fix now that it has been merged. @FlexW : need help?

@FlexW
Copy link

FlexW commented Oct 21, 2021

@Thatoo please create a new pull request with the change.

@mgallien mgallien added this to the 3.4.0 milestone Oct 21, 2021
@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 21, 2021

Here it is : #3916

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.

7 participants