Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1598934 - onedrive: Switch to more active upstream
Summary: onedrive: Switch to more active upstream
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: onedrive
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zamir SUN
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1692994
TreeView+ depends on / blocked
 
Reported: 2018-07-06 23:56 UTC by alex.braunegg
Modified: 2019-04-03 03:22 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-12-04 15:11:00 UTC


Attachments (Terms of Use)

Description alex.braunegg 2018-07-06 23:56:08 UTC
Description of problem:

The 'skilion' github branch is not the latest and contains a significant number of defects most notably unable to handle the OneDrive API change that generates the error 'Key not found: lastModifiedDateTime' for OneDrive Business Accounts. The 'skilion' code appears to have been abandoned by the original author and contains numerous other issues that have not been fixed.

If you wish to use the latest code which contains numerous fixes and enhancements you need clone from here: https://github.com/abraunegg/onedrive

Comment 1 Thomas Drake-Brockman 2018-07-07 08:40:55 UTC
Hi Alex,

I'm aware that your fork is more recently updated that the original fork, and I have been considering which is best to package. Very keen to explain a bit of my thinking, and happy to hear your thoughts:

1. The primary reason you give for changing fork (it seems) is that @skilion's fork is broken on OneDrive Business. It's worth noting that the Fedora package clearly states that "OneDrive for Business is not supported", something I reviewed when I last updated the package and concluded that supporting Business was still difficult at this point.

2. While @skilion's branch has been less frequently updated, it also seems more stable. There have been far more new features implemented on your fork, which gives me less confidence in it being 'tried and true'. The fork currently packaged meets the needs that lead me to packaging the client initially, and I'm not inclined to update for updates sake.

3. It appears me me that some of the changes that you've made in 1.1.2 are breaking changes (e.g. changing CLI flags: --synchromise vs. -m) even though you've only incremented the patch number (rather than the major or minor version number). This also leads me to be cautious with regards to stability of your fork.

Hopefully you can appreciate the reasons above, which have lead to my decision so far to remain using @skilion's fork. That said, it's definitely the case that I have concerns regarding how well maintained @skilion's branch is.

Would you be happy to address the above and see if we can address the issues that I am concerned about?

Cheers,
Thomas

Comment 2 alex.braunegg 2018-07-07 21:45:20 UTC
Hi Thomas,

Below are my responses to questions:

1. Skilion's fork has been broken in many many aspects - not just OneDrive Business but for OneDrive Personal as well. Whilst it 'may' have worked for some users, the skilion code contains ~20+ serious defects that prevents the client from working correctly - items such as:
* Correct conflict resolution handling (Skilion #171, #212, #243, #265, #338, #343 & potentially others)
* Fix usability defects (Resolve Issues #73, #121, #132, #224, #257, #294, #295, #297, #298, #300, #306, #315, #320, #329, #334, #337, #341)
* Correctly handling a 'bad' response code from OneDrive & not crashing in many places (too many individual issues here to list - 5xx, 412, 409 errors and the like)

Additionally, the Skilion branch was rebased / refactored in June last year where OneDrive for Business support was flagged as being supported in the readme (https://github.com/skilion/onedrive/commit/f00a80bcac8d6b66ebd8d53c31d2fc70dddc9aac#diff-04c6e90faac2675aa89e2176d2eec7d8) however this futher introduced issues where OneDrive Business has more strict checks around file / folder name types which were never implemented. To date all of these OneDrive Business issues have been addressed. All those checks have been implemented, tested & validated in my branch.

There is still 1 defect I am trying to close out that deals with OneDrive Business syncing and that is how the quickxorhash is being generated - see my branch #23 for more details. It is an edge / corner case, but regardless the computation of the hash should match what OneDrive creates 100% of the time & it does not.

2. I disagree with your statement about being more stable. Simply look at the issues list which grows steadily for the skilion branch vs when people switch over to my branch their issue is resolved & fixed. If my branch were unstable, then I would have defects / issues being raised at a similar rate - which I do not. I have also been making these fixes in Skilion #82 and later in #314 since July 2016 - addressing and fixing the code issues that the skilion branch inherently has. Additionally, the skilion branch does not contain any fixes for the OneDrive API changes that have occurred - further breaking OneDrive Personal and OneDrive Business accounts.

Additionally, I am also working on getting a correct test suite configured so that each commit when compiled, is run through a series of automated functionality tests against actual OneDrive Personal & OneDrive Business accounts. Today, all code passes through Travis CI for compilation checks as a start.

If you have concerns re the stability of my branch - run it through your own testing & then run the exact same tests against skilion & compare. If you do find an issue, please log an issue item & I will look at it.

3. The --synchronize flag to actually sync is not a 'breaking change' but I can see why you may think that. It actually is an alignment change to usability as for example - if you want to run it in monitor mode - pass the monitor flag - so this actually aligns the client to that usability by if you want to manually sync use the sync flag. Having an application 'blindly' sync when you run the application is not the way it should operate. Have a look at rsync, google drive client, aws sync clients & so on - all require a flag to perform a sync operation of one sort or another & not blindly just sync. This sort of 'functionality' caused one user of the Skilion branch to loose all their data on OneDrive because they had not mounted their drive - see Skilion #317 and #8 in my branch where I have provided a fix and resolution for that specific issue.

In terms of other features added (one way sync etc) these 'features' use the exact same code functions as a two way sync. They simply call only one of the functions to get the task accomplished, without performing the other side of the equation. If there was an issue with that code, it would have already been highlighted by the users performing operations with my code.

In terms of your concerns around how well maintained skilion's branch is - it's not just the code that has issues, but the responsiveness to users questions, issues and the like. Since June 2016 I have noticed very minimal response to any question leaving users to try and work it out themselves - that is not a very good end user experience from the original author.

Comment 3 Thomas Drake-Brockman 2018-07-09 13:25:12 UTC
Thanks for the extra detail Alex. I'd like to be able to use your branch, but I do think it's important that users of the package can expect that the program will behave the same way between patch releases at least.

Given that you're done a fair bit of work on the project since forking from @skilion, would you consider releasing a version 2.0.0, or at least 1.3.0? You might even adopt SemVer (https://semver.org/) from that point onwards to ensure that users don't get any unexpected changes.

People do generally expect to be able to upgrade their packages and have their scripts continue to work as before - which might be a bit different for people who are using your repo directly, and recompiling from source to upgrade.

If that's acceptable to you, we might have a good path forwards. Let me know your thoughts.

Comment 4 alex.braunegg 2018-07-09 19:39:12 UTC
I have no issue in flagging a new version due to the number of changes - I was hoping rather that the original maintainer was going to at least 'sync' in the fixes I have done which is why I had not increased the versioning beyond point releases.

I actually have a release ready to go now - there are a couple of tidy up items I am doing, then will flag a new release.

Comment 5 Thomas Drake-Brockman 2018-07-10 05:19:47 UTC
And what about adopting SemVer? Will you accept a PR for that?

Comment 6 alex.braunegg 2018-07-10 05:29:16 UTC
I am already using the SemVer conventions for the releases & will be following this a lot closer now that I have released the v1.4.0

Comment 7 Thomas Drake-Brockman 2018-07-10 06:01:34 UTC
In that case you should perhaps go for version 2.0.0 next? There are breaking changes since 1.x.x (however well intentioned) so the major version number should be bumped up.

Comment 8 alex.braunegg 2018-07-10 06:25:09 UTC
The v2.0.0 will be for major change that is planned around fixing a number of issues with monitor mode, Business Shared Folders and the like which will (at this point in time) need quite a bit of a heavy re-write of sections of code.

The v1.4.0 gives you a chance to signify that there are functional alignment changes in the code which is what the `--synchronize` flag does.

Comment 9 Thomas Drake-Brockman 2018-07-10 06:32:41 UTC
A quick SemVer recap:

>Given a version number MAJOR.MINOR.PATCH, increment the:
>
>    MAJOR version when you make incompatible API changes,
>    MINOR version when you add functionality in a backwards-compatible manner, and
>    PATCH version when you make backwards-compatible bug fixes.

Given that, it would be best to go for 2.0.0 ASAP, given the changes that have already been made. Internal re-writes don't need to be major version changes if the API stays the same.

Comment 10 Zamir SUN 2018-07-10 08:22:42 UTC
Changing version to Rawhide as we never expected to push major changes in released version.

Modifying the bug summary to make it easier understandable.

Personally, make @skilion accept the changes is always a preferred way. However if this is not easily acceptable, I prefer to switch upstream when updating to new major versions (like Thomas mentioned in comment 9).

Thomas, if you are going to update the package when it's ready, you can take the Assignee of this bug.

Thanks.

Comment 11 alex.braunegg 2018-07-10 09:36:28 UTC
version updated to v2.0.0

Comment 12 alex.braunegg 2018-07-10 10:18:43 UTC
Note: Just as an FYI for the spec file / package, due to the database issues uncovered & fixed - the 'items.sqlite3' file which stores the local cache of files & their sync state should be removed & allowed to be re-created when the application is next run.

Comment 13 Thomas Drake-Brockman 2018-07-21 11:18:06 UTC
Thanks Alex. Couple of queries:

1. I just checked your CHANGELOG.md and there are no entries since 1.1.1 - is this intentional? 
2. Would it be possible to have the incompatible items.sqlite3 recreated automatically? Having a package upgrade go messing around deleting files in user's home directories is probably a non-starter.

Comment 14 alex.braunegg 2018-07-21 21:37:46 UTC
In relation to #1 the changelog.md file - oversight rather than intentional.

In relation to #2 - there is no way to determine a bad 'items.sqlite3' by running any sort of query. You should be cleaning up this file anyways on application removal / upgrades anyway as all it is, is a state file of what has been synced & what has not. The 'items.sqlite3' file gets automatically created if it does not exist already. Additionally, the code changes made validates if the file is already present on OneDrive with the same hash before it determines if it should upload the file as new or changed. This is a massive internal change to how the skilion code operated where if 'items.sqlite3' it would treat the file as new and upload blindly.

Comment 15 alex.braunegg 2018-07-22 05:16:22 UTC
Additionally - simply renaming this file will also have the desired effect of the application re-creating this file.

Comment 16 Thomas Drake-Brockman 2018-07-22 06:25:37 UTC
Alex, Fedora packaging guidelines explicit prohibit a package meddling in the user's home directory: https://fedoraproject.org/wiki/Packaging:Guidelines#No_Files_or_Directories_under_.2Fsrv.2C_.2Fusr.2Flocal.2C_or_.2Fhome.2F.24USER

You'll need to update your project to detect when a user is using an incompatible cache and upgrade to recreate it when that occurs. For example, you could store a schema version number in the SQLite database, and recreate the cache when this version number is mismatched.

Comment 17 alex.braunegg 2018-07-22 06:55:14 UTC
Well I guess this is an impasse then.

Feel free to submit a PR when you work out how to resolve it.

Comment 18 Thomas Drake-Brockman 2018-07-22 09:10:36 UTC
Hey Alex, I'd encourage you not to see it as in impasse.

Fedora guidelines are fairly clear - and I think they're in place for good reason. For example, some users might have their home directories mounted from remote servers on login, or have configured custom paths for their configuration files. It's not the responsibility of the system package manager to handle user configuration, even if it was possible to reliably do so.

Now, I do genuinely want to package your fork and version 2 for Fedora, but I need to know that I'm not going to cause issues for users, so the following would be helpful to work through:

1. What would currently happen if a user ran onedrive 2 with an old items.sqlite3 from skillion's version 1? Graceful warning/error? Crash/segfault? Data loss? It might be that the user can just be prompted to remove the file.

2. The ideal user experience would be to deal with removing the old cache automatically. This might be as simple as renaming the cache to items2.sqlite3 going forwards, and ignoring any items.sqlite3 that may have been created by a previous version of onedrive. You could also embed a version into items.sqlite3, but that might be overkill.

I am genuinely keen to find a resolution that will keep users happy, but please realise that I'm constrained by Fedora's packaging guidelines.

Comment 19 alex.braunegg 2018-07-23 07:56:50 UTC
In regards to the DB issue, I believe this is solved now with this commit:

https://github.com/abraunegg/onedrive/commit/deabc0d21230e6ce6e79c37bb4b47f02b5990735

This change looks for the DB version match, and if mismatch, drops the table & re-creates it.

To answer your main question above:

When using the older database - due to the incomplete error handling in the skilion code, it was found that items were being stored in the database which had incomplete item details. When using an 'upgraded' client but using old database, the client would complain about parent items not being in the database and quit.

Comment 20 Jan Kurik 2018-08-14 09:55:19 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 29 development cycle.
Changing version to '29'.

Comment 21 Zamir SUN 2018-11-30 12:07:35 UTC
Hi Alex,

I am now working on switching the branch now. While I see there are two things that worth improving before I can get this built in Fedora. I've sent a pull request to you. Once this pull request is merged (or similar fixes implemented), I will build it in Fedora.

JFYI, as now there are outstanding changes to onedrive comparing with the old upstream we are using, I am planning to get this in since Fedora 30.

https://github.com/abraunegg/onedrive/pull/259

Comment 22 Zamir SUN 2018-12-04 15:11:00 UTC
Now build in Rawhide.
I would appreciate if anyone can test it.

Thanks Alex for the actively maintaining the onedrive project.

Comment 23 Zamir SUN 2019-04-01 05:59:24 UTC
*** Bug 1692994 has been marked as a duplicate of this bug. ***


Note You need to log in before you can comment on or make changes to this bug.