nextcloud / client_theming

:computer: Nextcloud themed desktop client - Moved over to https://github.com/nextcloud/desktop
GNU General Public License v2.0
198 stars 87 forks source link

Update client to release v2.3.3 #206

Closed tsmith512 closed 6 years ago

tsmith512 commented 6 years ago

Would resolve nextcloud/client_theming#202 and addresses nextcloud/server#5238 . The latest Nextcloud server version cannot sync files with client version 2.3.2 because of changes to WebDAV chunking. To resolve this issue, I updated the client submodule to origin tag v2.3.3 and built a Windows package with the instructions from nextcloud/client_theming README.

I had a Docker build/installation error: (resolved in dd25240)

→  docker build -t nextcloud-client-win32:2.3.3 client/admin/win/docker/
[ ... ]
Step 7 : RUN zypper --non-interactive --gpg-auto-import-keys ar http://download.opensuse.org/repositories/windows:/mingw/openSUSE_42.1/windows:mingw.repo
 ---> Running in cdcadf211b75
File '/repositories/windows:/mingw/openSUSE_42.1/windows:mingw.repo' not found on medium 'http://download.opensuse.org/'

Abort, retry, ignore? [a/r/i/? shows all options] (a): a
Problem encountered while trying to read the file at the specified URI:
ABORT request: Aborting requested by user
The command '/bin/sh -c zypper --non-interactive --gpg-auto-import-keys ar http://download.opensuse.org/repositories/windows:/mingw/openSUSE_42.1/windows:mingw.repo' returned a non-zero code: 4

To resolve, I changed the Dockerfile in the client submodule with an updated path on the openSUSE repository (added in dd25240, same as upstream solution for owncloud/client#5900)

diff --git a/admin/win/docker/Dockerfile b/admin/win/docker/Dockerfile
index 8e40a49..1629b97 100644
--- a/admin/win/docker/Dockerfile
+++ b/admin/win/docker/Dockerfile
@@ -8,7 +8,7 @@ ENV HOME /root
 ENV REFRESHED_AT 20160421

 RUN zypper --non-interactive --gpg-auto-import-keys refresh
-RUN zypper --non-interactive --gpg-auto-import-keys ar http://download.opensuse.org/repositories/windows:/mingw/openSUSE_42.1/windows:mingw.repo
+RUN zypper --non-interactive --gpg-auto-import-keys ar http://download.opensuse.org/repositories/windows:/mingw/openSUSE_Leap_42.1/windows:mingw.repo
 RUN zypper --non-interactive --gpg-auto-import-keys ar http://download.opensuse.org/repositories/isv:ownCloud:toolchains:mingw:win32:2.3/openSUSE_Leap_42.1/isv:ownCloud:toolchains:mingw:win32:2.3.repo
 RUN zypper --non-interactive --gpg-auto-import-keys install cmake make mingw32-cross-binutils mingw32-cross-cpp mingw32-cross-gcc \
                       mingw32-cross-gcc-c++ mingw32-cross-pkg-config mingw32-filesystem \

Future upstream releases will have the fixed repo location.

The resulting Windows binary works fine for me and successfully synced the files that were giving me the Precondition Failed error.

Disclaimer: I haven't built or tested binaries for other platforms.

tsmith512 commented 6 years ago

Automated testing fails in the Debian build job (https://travis-ci.org/nextcloud/client_theming/jobs/280040798) when trying apply the patch fix-application-icon-name.patch, but I can't figure out what it's trying to patch. I followed the build instructions on an Ubuntu box and didn't get an error, so it looks like it's part of the Travis build script and that the basever may not be set correctly:

→  grep -ri basever
linux/debian/travis-build.sh:    read basever kind <<<$(linux/debian/scripts/git2changelog.py /tmp/tmpchangelog stable)

Looks like it's just downloading ownCloud's client and extracting it, but it's either not downloading the right version or 2.3.3 hasn't been put up on the PPA. At this point, I'm not sure what to do since I don't know the dev process on this project. I'm happy to keep working on this, but I may need a little hand holding. As a web developer, this is outside my usual area.

juliushaertl commented 6 years ago

Looks good to me. Maybe @ivaradi can help on the debian build error.

ivaradi commented 6 years ago

fix-application-icon-name.patch.txt This a new version of the icon name patch, which works with 2.3.3.

ivaradi commented 6 years ago

The build fails this time because it tries to apply the new patches on the old (2.3.2) Debian upstream package.

The Debian build determines the "main" version (e.g. 2.3.2 or 2.3.3) by checking the latest Git tag, which is "v2.3.2" on this branch. Hence it downloads the 2.3.2 upstream package, and tries to use that instead of creating a new upstream package for 2.3.3.

So the solution would be to apply a tag named, e.g. "v2.3.3-beta" to your original patch.

tsmith512 commented 6 years ago

Applying a tag v.2.3.3-beta to my branch in my forked repo didn't trigger a rebuild, so I made a minor typo fix in 604db1d to rerun the tests. Is there a way to override what this script would use to pull its binaries? Since tags don't move or cross forks, would a branch name or config file make that more stable?

The other thing I want to keep an eye on is that Dockerfile change I had to make for the mingw libraries.

ivaradi commented 6 years ago

Well, the branch name is also lost for a pull request. A config file might be considered, but this version information is not used for deciding which upstream package to download only.

There is a script that processes the Git log and produces the changelog for the Debian package. The Debian changelog also includes the version information and the Debian build tools use the changelog (its topmost, i.e. latest entry) to determine the version of the package. To be able to (re)produce the older changelog entries, it is important to have a sequence of properly named tags attached to the right commits. The actual version (used among other things to select the upstream package, if any) is a by-product of this script.

That said it would be possible to have an optional config file which tells the script, e.g., that from commit CCCCCCCC on behave as if it were tagged TTTTTTTT. This way forks could be supported. Once the fork is merged, we could create a branch on this repo that would create the tag and remove the config file. This is a somewhat roundabout method, but might work. I will look into it today in the afternoon or the evening.

ivaradi commented 6 years ago

I have created a pull request on your repository with the config file support. Please, apply it and let us see if it works.

tsmith512 commented 6 years ago

Nice work! Thanks, now tests are passing.

tsmith512 commented 6 years ago

I still can't build a Windows binary without that patch on the Dockerfile in the client repo for the mingw location, looks like Travis may not be testing that build.

ivaradi commented 6 years ago

That's correct, Travis does not build the Windows binary.

However, I think that would be a nice addition. I would suggest adding a new build type for that and implement a new build script, which is called from the main one (linux/travis-build.sh). Could you take care of it, or shall I find some time to do it? It would also be nice if the resulting binary could be uploaded somewhere.

As to the Dockerfile patch: as the file comes from the upstream ownCloud sources, they should be convinced about making the change. Otherwise the said build script could patch the file until the upstream is corrected.

tsmith512 commented 6 years ago

Added the Windows test and it passed. Also I found the MinGW location issue, which upstream fixed in owncloud/client#5900, it's just not in the 2.3.3 tag. I've added the patch as you suggested and documented why it's there. Hopefully in the next release we can dump it.

The Windows build outputs an installer for 2.3.3.1... Should I add the $basever as an argument on the build.sh that gets passed to the Docker instance? From there, I don't know what to do about publishing the installer from Travis directly.

ivaradi commented 6 years ago

I am not sure where the .1 comes from, but it may be the -DMIRALL_VERSION_BUILD=1 argument passed to cmake in build.sh. So perhaps passing $basever may not work anyway. BTW you can remove all the PPA- and OBS-related stuff from the new travis-build.sh script.

Regarding publishing: Travis does not really know about any publishing methods. The Debian packages are published using the ordinary commands available on Ubuntu. The current Windows binary can be downloaded from https://download.nextcloud.com/desktop/releases/Windows/Nextcloud-2.3.2.1-setup.exe, so perhaps there is a way to upload the installer there, but I do not know about it. (As you can see, this installer's version number also includes the .1, so it is probably not a problem.)

tsmith512 commented 6 years ago

So that .1 comes from -DMIRALL_VERSION_BUILD=1 in build.sh. I tried changing it to the basever variable, but got a build error that there were "too many decimal points". Leaving it blank resulted in an error, too. But when I changed it to 9, the installer I got is called Nextcloud-2.3.3.9-setup.exe. Looking back to the README, it's just the build number that shows up here:

image

I don't think Travis needs to know that, but it should increase for every minor/bugfix-level release for the auto-updater, I guess.

tsmith512 commented 6 years ago

Thank you for that explanation, that context helped me read through what was included there. I've removed those lines as well. So at this point, what do you think is left to get us to a release? I'm seeing more pings on the broken sync in #202. Thankfully the dev build has been syncing big design working files successfully for me for a week.

bloukrans commented 6 years ago

Hello tsmith512, thanks for solving this issue. unfortunately the built file is still not available. can you upload it to a temporary place for all the other using suffering from "Precondition Failed" error? thanks

tsmith512 commented 6 years ago

@bloukrans Howdy! Per my comment on issue 202, I cannot release an official build and using an unsigned installer is at your own risk, but I have uploaded my build of the Windows installer for temporary use until this is sorted. Is this the platform you need, or are you looking for a different one?

edit: In another comment on that thread, a user mentioned using a previous version of the official Mac client to resolve this issue.

(also edit: new dev build, from 2.3.2.1 which was improperly versioned, to my unofficial 2.3.3.9)

bloukrans commented 6 years ago

Hi, thanks for your fast revert. Yes windows platform is the most critical for me...I did download your mentioned build earlier - but this file is the signed 2.3.2.1 build still giving me the error message "precondition failed (An if-match header was specified and the resource did not exist)". I also saw downgrade option on Mac, still I'd rather move forward with this issue...

tsmith512 commented 6 years ago

@bloukrans Ah, yeah that was an early dev build which did include the wrong version number, though it should have fixed the issue. Here's what I'm currently running on my machines: https://tsmith-fileshare.s3.amazonaws.com/Nextcloud-2.3.3.9-development-setup.exe still a development build and still comes with the warning about downloading installers from strangers, but it fixes the precondition failed error and includes the icon fix. Make sure you let the installer uninstall the existing version first. Please let me know if this solves the issue, since I don't know how many others are using this, if any.

bloukrans commented 6 years ago

Hi Taylor, at a first glance, everything looks alright now. Thanks a lot for this!

Do you know whats holding nextcloud back from finally resolving this issue? is it a major rewrite coming for v13 with end2end encryption?

at the moment, the client solution is far from good - both android and windows (mac i have no idea...) have to use beta versions in order to function properly...