Folder creation broken in Android app

[Catfriend1]

Hi there.

I’m so sorry the transition to the new folder type supporting v14.49 syncthing version broke the part of new folder creation on the android app. I’ve immediately put up a PR to fix the problem, so please hold on and don’t report this bug again. Link to github ( https://github.com/syncthing/syncthing-android/pull/1191 )

Until the fix is released, you can work around this by toggling the “Send Only” slider “back and forth” before saving a folder you’d like to create.

Changing existing folders still works properly.

Kind regards Catfriend1

EDIT: 389 (and counting) sessions were affected. Feedback on GPlay confirmed the bug.

[Catfriend1]

While the fix got merged in the meantime, I think @AudriusButkevicius we should continue the release management discussion on the forum and not in a single PR.

@all: If someone can help us with creating unit tests that somehow no one did since approximately 3 years for the android app, this would be VERY awesome and appreciated. (see https://github.com/syncthing/syncthing-android/issues/385)

Quotes: AudriusButkevicius commented an hour ago

So much for testing I guess.

I think you should start writing automated unit tests, because as we can see manual testing doesn’t scale and catch everything.

AudriusButkevicius commented 20 minutes ago

Talking about software development in a professional fashion, I doubt you’ll find a place where unit tests are optional (unless it’s a complete fucking cowboy place that hires anyone and doesn’t care about going out of business). Writing tests is part of a developers job.

Sure, it’s open-source, we can be sloppy, but I can just not merge stuff until it has some reasonable unit test coverage to prove that it works.

I’d rather have no improvements, over improvements that might just break the app all together just because we don’t feel heroic when we write tests.

Catfriend1 commented 3 minutes ago

@AudriusButkevicius I see your point. It’s like you stated, we are no business so if something is fucked up it gets fixed, the sooner the better! If you can’t live without tests (as syncthing core has them), feel free to contribute them. This is not a valid argument putting the work on me. I think we both wouldn’t be lucky if a fork has to come up to have an easier way to push app development forward and then gets out of sync so multiple app’s would coexist. There are much prettier ways as making a better release and hotfix plan which you disagreed?! before. So this is my suggestion:

• We push everything after reviews and incremental testing took place forward to the master. That’s consuming a lot of time and from my side is okay as we currently handle it.
• The master regulary gets a new beta build pushed to google play “weekly” (+/- some days okay of course as some of us might be busy)
• The “third” beta in the cycle will be uploaded to transifex. (I assume this is roundabout the third week after the last release) People can start translating new stuff to eliminite this “german/english” artifacts in the UI we currently experience.
• The “fourth” beta pushes a new syncthing core version in (when its ready in its regular release cycle) and gets uploaded to GPlay (most translations probably done until then) Beta, because the current handling using the release script to automatically bump syncthing core’s version is not sufficient to eliminite wrapper compatibility errors before release if any come up. I guess “you” (or anyone else) didn’t test the release version after syncthing 0.14.48 got 0.14.49 compiled in before pushing it to google play. It was me doing the testing on my private build server as the master wasn’t even linked to 14.49-rc.4 at an pre-release point in time.
• If the “fourth” beta doesn’t turn out any high-priority issues impacting the major use of the syncthing app, it should get pushed to Google Play as a release after about two weeks.

To sum up: Approximately six weeks after the last android app version release we would finally push out a new release to google play.

If we have an urgent problem like with 0.10.12 and the folder creation, we should discuss if it’s okay to do a hot-fix release only cherry-picking the most important commits from the master related to the hot-fix (as the master might have more merges in-between already).

I’m writing this in the hope “we” find an agreement that empowers the work together without creating too much overhead as we haven’t much active contributing people on this repo at the moment. I would agree on putting more rules on this repo when and how changes get in when the major downsides of the app got fixed and we have more active contributors.

[calmh (Jakob Borg)]

Adding tests with/for new features and bugfixes is nice. I doubt anyone expects you to produce tests for all legacy stuff. Generally, few people here should throw any stones in the testing glasshouse…

The Syncthing integration tests are unfortunately of limited value, despite efforts to clean them up. Unit tests, and hence unit testable code, is where the bang for the buck is.

(Not my project, not trying to tell you what to do, just an opinion.)

[Catfriend1]

@AudriusButkevicius Could you please make a bugfix release 0.10.13 as there are more than 650 users affected which cannot access basic app’s functionality as creating a new folder is broken (the existing workaround to move the toggle forth and back is not obvious to users). Despite our discussion on strategies, tests and release cycles last night in the PR, I think we owe that to the users regardless whom or which change/testing is to blame.

Reading between the lines in the PR’s discussion what you’ve said I feel somehow you don’t care much about the android repo for the reason it costs you a lot of time. Indeed you do care very much according to the review activity. Please let me know if I can assist in PR merges and release management (seen my milestones in there?). That should not happen without reviews of course, but to lower workload on your side. Unfortunately I don’t everything at hand at the moment to make an urgent bugfix release to google play (preferably the beta branch permission and proper signing keys).

Absolutely understood your point brought up in discussion why the project shouldn’t move on too quickly with new features as that not only requires my availability but also yours. “It’s not enough to fix broken things quickly in code, someone has to test and release those fixes and monitor what people/gplay reports say on it”. So I’d be happy to assist.

Current status on gplay reports for the current version 4145:

Related user feedback public on google play:

[Sturmlicht]

Hello there,

I am following the improvements of syncthing-app for a while already. I hope this is the correct thread to post this as discussions are spread.

What I currently think is reality: There is two people working on the syncthing wrapper. Catfriend and Audrius. Impression I gained from Audrius so far: He pushes the merge buttons and also combines translations and commits from Catfriend to the google play version. Often enough translation for the latest versions are missing as they are pushed too early to the live version. He does not seem to test the app before he pushes it to the live version. Often enough his responses to others could be improved. However, we all know he is also helping calmh to produce the real syncthing. So he must have a lot on his desk already and working on this wrapper is voluntary, so it is fine to not “manage” it more then he wants to. But I think some help for him would be welcomed. Impression I gained from Catfriend so far: With him joining the project, the app gained a lot more stability and improvements. He is not shy to acknowledge when he is not sure how something is coded, from his first baby steps in android-app world, he evolved greatly and I think it is save to say, that he does not make beginngers-fails anymore. So far I have seen him being responsible and responsive to all the problems (not only the ones he produced, but also stuff that was around for a long time already), besides that he tests his stuff and always makes a note until which point he tested, so actually it is easy to follow. However, his reach is limited to the github world, so not many people on google play will ever see his documentation. This gets even clearer by recent events. Let’s get to this now.

There is currently three topics going on:

1. Hotfixes should be applied as soon as they are ready to a live version that has a major problem
2. QA is week as there is not enough developers, so a solution is needed
3. Management of PRs, QA and Release could be improved.

Let’s elaborate on them a bit more:

1. If it got clear, that the live version has a problem, and the hotfix is simple and can be tested quickly. Then it should be pushed to the live version on all platforms with a proper announcement. Google Play seems to be having an issue in this regard. It’s just a guess but with all those android users it might be save to assume that a lot of people get the app via google play, that also means that when issues get introduced to the live version you guys lose people on Google Play, and then syncthing reputation suffers from this. -> The latest hotfix is available quite a time on github however it is not yet pushed to google play, I see no reason for this delay (however I do not know how much effort it is to push the app to google). Audrius and Catfriend seem to be both aware of the hotfix, however Catfriend seems to be not able to push it to google while Audrius might have other things to do right now.
2. You cannot assume that one person � even if it is the programmer � can find all the issues, not even by a testing routine. Big companies have a QA-department for this. As this team is too small I am sure there is currently no way for this, besides that, it is quite time consuming. However, I see users being responsive if asked for describing the issue and even helping out to find the bug. I think it was already mentioned somewhere, but here again: Google Play has a BETA functionality, where registered users can help out testing new features and commits. But those pushes should be done several days before a version goes live to the “all user branch”. This leads me to my third point.
3. Management of when new features go into the live version should be improved. I don’t want to elaborate on all the issues that the project currently has, in terms of new features, translations and bugs. I would like to point to a solution that could be done easily and should be tested if it is found valuable by you guys. It is quite in line with what I wrote previously and I am sure you could already guess it. And you guessed right: Use the Users. use the Beta branch on google play. As one person cannot always be available to push versions to Google (Live or Beta) at least two persons should have the rights to do so. For every feature or fix, the Beta branch should be used SEVERAL DAYS before it goes to the live branch. Be communicative on Google play and write a short text of “what has been improved”, “known issues” and maybe even “soon to come features”. I am not sure if Catfriend would like to have the responsibility and would be kind enough to help audrius out on those tasks. But I see that it would greatly help the reputation of syncthing and the app.

Thanks for reading this far. Sturmlicht

[calmh (Jakob Borg)]

The (any project’s) release process needs streamlining and automation. I’ve done a lot of that for Syncthing but there’s still a number of steps, button presses, scripts, and commands each of which sometimes goes wrong and requires hands on to resolve. So I don’t know how this is on the Android side, but I can see how inducting a new release manager can be tricky, and two releases (beta + stable) might be twice as much work as one.

That said, it would seem like the ST-Android project could use a similar approach as ST itself - perhaps release a beta together with the corresponding ST release candidate, and then make that the production build with the corresponding ST release a month later. That’d catch breakage from API changes etc a bit earlier in the beta at least.

[AudriusButkevicius]

Sure the release process can be improved, but I don’t want to be in the boat of having to be there every time calmh releases a syncthing rc. If we can automate rolling to beta everytime syncthing releases and rc, and then at some point releasing that (and not whats on mastern like we do now) to prod, would probably be better, but its still shit, because there are barely any users on beta track, and I don’t expect them to keep opening the app, adding folders and what not on a casual basis, as this essentially a setup once and forget app.

So fuckups will still slip through, which could pontentially be caught by unit tests.

I don’t use the app, nor do I use syncthing, so I am in no way going to be on the manual testing path.

I have not released a hotfix because I am waiting for more dead bodies to come to the surface, once we know we got all of them, I’ll make a release, probably this evening. I don’t want to be releasing hotfixes every day.

[calmh (Jakob Borg)]

It sounds like it may be worth it to take the effort to hand over the baton for day to day releases, with as much as possible automated of course.

[Catfriend1]

@calmh I’d be there if you need me to do the “RC” releases on the beta channel. Adding the “release to beta channel” to the current (or a second alternative) release script shouldn’t be so hard as long as we’ll keep the APK’s version number increasing. GPlay’s beta channel currently has 885 testers. That means, if we publish a new beta build, the play store offers the user that has signed into beta himself the beta build looking like it was a release on the play store UI. Users that update automatically get the beta build automatically. This eases getting feedback from users as they wouldn’t need to be asked to start the app and do something with it for testing - they will use it as google play store “forces them” according to their own opt-in. Our beta users stack is sufficiently large I think. We’ll have 31069 active installations of the app on the release build according to the gplay console. So beta users make a solid 5 to 10 % out of it. That’s because the numbers cannot be compared directly as installations != beta users (a user may have more than one device!). If it would be granted, I’d like a “beta branch” on github where I can move on things more quickly and also pull in a current syncthing rc (without usage reporting of course) if needed for example on folder type transitions. Commits on the beta branch can then be taken over into the release branch if users are happy with the gplay beta version and bugs got fixed. With this strategy, the release branch would need less caring of @AudriusButkevicius as he’ll just get well tested stuff and can go on building releases from that.

[calmh (Jakob Borg)]

I do nothing useful on the Android side, I just provide opinions - for free.

[Catfriend1]

@AudriusButkevicius Related to your hotfix release 0.10.13 and commit ( https://github.com/syncthing/syncthing-android/commit/a9a863ace95174d5742dc38e90280b5949dc1b79 )

First, thanks you took care about fixing the russian encoding problem by a workaround. I said, I disagreed with your commit without review to the master and pushing this immediately public.

Did you investigate this issue ( https://github.com/syncthing/syncthing-android/issues/1198 , https://github.com/syncthing/syncthing-android/pull/1199 ) deeply as I did before changing the code, honestly?

Did you know your fix is putting phone at danger to be deleted accidentially as the code now (according to my reproduction with the folder name the user in issue #1198 mentioned ) fails to create the test file and then a delete command is put into the shell with all that encoding problems? I f**** up some data on my phone yesterday investigating this issue as I had an un-deletable, wrongly encoded folder in my filesystem during testing different cyrillic folder names and also tried the “rm solution” to get rid of them as … suddenly one cyrillic name ended up in the encoding failure and rm began a file deletion in the root. I just needed time to find a solution and already said I’d fix this. I requested you to merge my workaround PR #1199 (dirty workaround not doing shell stuff on foreign language and returning true which restored UI functionality to that of earlier releases) until my proper fix is ready. I’m currently testing this and this time is really needed to don’t get any more bad. Why didn’t you just make the hotfix release containing the urgent PR about the topic of this thread? We just had one(!) user reporting the encoding issue on folder creation using cyrillic language. I can’t get it, sorry. You were the one reviewing very detailed my PR’s and yourself don’t get into review? @imsodin also offered help, so there are at least three people available taking @capi and me into account.

We really should get better! Related to your more and more “test would’ve helped” comments, yes maybe that’s true, but a human testing also has a value and both testing strategies are nothing if our release cycle is react instead of act in proper and timed order.

P.S. I’m reopening issue of the user (as you closed it and provided a workaround yourself rejecting mine with the need of a proper fix). I’m reopening my workaround PR and titling it “WorkInProgress” offering the proper fix I found some minutes ago. I’ll tell you when I’m done with testing on my phones as I’m not allowed to merge/release. Then, my suggestion is to make 0.10.14 BETA with the proper fix for the encoding-in-shell problem contained.

P.P.S. As I’m willing to invest time, things would go faster if I could act like @Nutomic did it in former times. When fix>beta>fix>beta> … cycles would be over and quality assured together with the beta users, this can of course get the “long way”. Decide if you want it. Deal is I catch up the work “Nutomic formerly had less time and didn’t do” to be compatible with modern phones Android (especially 8+ where we now have most problems with the service background ANR timeout resulting in a “crash”), take responsibility for my changes on the beta channel and offer support, make some sensible approaches to restructure existing functionality in a new UI (including user feedback) and have more flexibility in trying code out with those ~ 800 users. (Not for the master, of course!)

[AudriusButkevicius]

This is not a significant enough issue that this needs a beta.

Your fix fixed symbols from one language, there are other languages that use non ascii characters on the planet, so it’s more like a workaround for languages that use cyrillic only, and all other people are left in the water.

Yeah, I guess I could have skipped it all together, but because we were addressing immediate reports by users, it made sense to have it part of the same boat.

Yes, I probably should have added the return at the top, but we’re past that point now, and I can’t change that.

The rm is not recursive nor it’s a force, and always ends with .stwritetest, so this “deletes everything at root” sounds implausible, as it would not match anything sensible.

I am all up for utilising the beta channel, but as I said, I don’t want to have to push a button every Tuesday, so some automation has to be developed.

[Catfriend1]

@AudriusButkevicius I noticed you still disagree with my latest fix (not workaround) in PR #1199. I’ve merged it as I’ve confirmed it will fix the cyrillic (and other languages encoding) problems too in the subshells as it uses a new class instead of the deprecated one from java that automatically handles stuff right (see java docs). I lost some files at root - sorry, with root I meant my sdcard root not os root. As this may happen to other users too on 0.10.13 I request 0.10.14. To save you the work, I’ve put the proper solution at the repo, ready to make a release. If you tell me what to do, I can push the button for you. If it turns out temporarily necessary - every tuesday.

Fix sending wrongly encoded commands into the unix subshell (fixes #1198)

[AudriusButkevicius]

So your changes are in master. As I said, I don’t think it’s a problem worthy yet another release, hence can wait out the a whole cycle.

I’ve raised some other points about passing unescaped user input straight into the root shell. This is not related to this issue, but it is an issue/oversight in general. We should either use some shell escaping utility function or back out subshelling user input.

I’ve enabled “no pushes to master unless merging a PR” and all PRs now require a review before they can be merged.

I guess we could create a release branch which essentially follows some commit on the master tree (so not actually a branch, just a marker), and every time the release branch moves, it rolls out a new beta.

Then once the beta has been stable and has not moved for atleast a week, we could promote it in gplay.

[Catfriend1]

@AudriusButkevicius As I said in the PR #1199, we DON’T pass user input into the subshell so the extra work is optional and gets required if we need to pass user input in the future.

Go easy on reviews. I’ll did and will do my best making good code. But be noticed that if I detect you waste my time on 5% important things that have no risk to break anything but you want from one “previously existing” copied code part a whole refurbishment (as you requested already some times) that’s not subject to my PR you’re getting nearer to the Catfriend1 fork!

So this is definitely new from the past minutes, agreed from my side but must be handled fair.

[AudriusButkevicius]

we DON’T pass user input into the subshell so the extra work is optional and gets required if we need to pass user input in the future.

[Catfriend1]

Okay okay, you got +1 because you’re right. Should we roll the dice which one of us should fix it and whats the planned work hours to do it? Better give the whole code to the security analysts at Controlware, Dietzenbach, DE and they’ll tell us which other weaknesses it had. I’d JUST intended to make syncthing more popular as solving major show stoppers which stop people (according to numerous posts, issues and feature requests) finding syncthing-android attractive. One main stopper was “it cannot sync on external sdcards but [third party app] can”. I wanted more users instead of less. That’s why I put the subshell permission detection stuff in disallowing people to (not knowingly) setup sendreceive folders where android restricts the access to readonly without telling that to the user.

I provide a fix for (estimated) 15% users locked out of the app and it gets to gplay 30 days later. This is quite doing the opposite. I created two PR with milestones, you didn’t care about it merging them at the proposed and explained-why times.

Turn round every cent and make one round in the circle after another, if you seem to block development, I now have enough thought about it to make the step to request a new reposistory admin and reviewer. One thats cool on people and says, its a minor thing, get it in. If it broke, and yes, everyone can fail including me and no machine or human test can prove something to be correct, it should be fixed. I definitely like having a person on the side where I can come “anytime” and tell “Sorry, something got broken, but I want to fix it.” Admitting this, should have consequences. They should be positive and not punishment as no one introduces problems willing to do so.

[Catfriend1]

@AudriusButkevicius: I tested your example on the current master with my latest fix included. Expecting the shell code would run if I add "; touch a; echo " in between and/or leaving the quotes out it didn’t run. So the new encoding handling class BufferedReader somehow has a side effect invalidating our attempt to execute an unwanted shell command. See screenshots, my log looks exactly like yours but nothing happened in the file system (even putting SELinux protection temporarily off).

[AudriusButkevicius]

I don’t think the buffer reader fixes it, it potentially gets interpreted weirdly by the shell, but because shell output is discarded it’s hard to say how it gets interpreted.

I think wrapping the path in https://source.android.com/reference/tradefed/com/android/tradefed/util/StringEscapeUtils.html#escapeShell(java.lang.String) should be enough.

I am not saying this is a matter of life or death that needs to be fixed now. All I am saying is that as it stands, it has problems. You disagreed, and I am just trying to provide evidence to back my claim. It’s not a personal attack of some sorts.

Pull request reviews apply to everyone, and I enabled them to have the same setup like we have in syncthing, where each change, including my own, would go through a second pair of eyes. There will be unreviewed pushes, but that should only apply to making a release, or an urgent (even if it’s a shitty) change like today which allowed to fix a minor bug with the new release.

[Catfriend1]

Thanks for the note on escapeShell. Noted it for a future contribution on my todo list.