munjeni / newflasher

Flash tool for new Sony flash tool protocol (Xperia XZ Premium and further)
336 stars 43 forks source link

Professionalize newflasher and prepare for Debian #9

Closed vog closed 3 years ago

vog commented 5 years ago

For Sony Xperia XA2 devices, newflasher plays an indispensable role in installing LineageOS, because the stock firmware of XA2 has a critical bug: When it is booted in SYSTEM_B, several sub systems like wifi and bluetooth don't work, meaning that after installation, the first OTA update of LineageOS (switching between SYSTEM_A and SYSTEM_B) makes the device unusable. Currently, the only known solutions are to either always install 2 updates in a row (which becomes annoying quickly over time), or to reflash parts of the firmware via newflasher. So if we want LineageOS to be installable long-term on XA2, we need to make newflasher long-term available.

This is why I would like to professionalize newflasher to reach a level of quality that it is acceptable by the Debian project. I plan to volunteer for the role as Debian maintainer for this package, but wouldn't be unhappy if somebody else wants to do that job. ;-) I already contacted the Debian AndroidTools team and they agreed to review and upload a Debian package for newflasher as soon as it meets their standards. They also performed a first quick review on this repository and provided lots of valuable hints about what should be improved. I'll try to fix each of those hints in the following issues and pull requests.

@munjeni Since you wrote in #3 that you are "not active here" anymore, may I propose that you create a new GitHub organization newflasher and transfer this repository over there? That way, you could add some trusted people as admins and establish a maintenance team, where each member could continue this project even if all others gets stuck in daily-life obligations.

munjeni commented 5 years ago

Hi, first of all thanks for contributing and for idea about newlasher on Debian distro. To be honest, I am bussy with private life, I spent a lot of time on xda and I simply have no more that free time for that hoby anymore, "I'm not active here" mean I'm not that active like before but that not mean I'm not active, I have some free time for example to accept some pull reguests but I have no free time for testing each, and I have no free time for making new organization, everybody is free to clone newflasher and start working on it on its own git accepting @c from newflasher.c . So if you need GitHub organization called newflasher you can do it.

vog commented 5 years ago

@munjeni Thanks for accepting the first batch of my pull requests.

I hope you did not get my proposal to create an organization wrong. Since you confirmed that your time is very limited, which is perfectly okay, it is even more important to create an organization now rather than in a year or two. My intent was not to put additional work on you. I just wanted to ensure that you can stay in full control over the whole process if you want. Since you don't, I took the freedom to create the newflasher organization myself, and of course I invited you to become a member with "owner" privileges of it.

There's one thing I can't do for you, though, which is the transfer of this project to the new organization. This is essentially just 3 clicks and 2 textfields for you, see also:

Of course I could fork newflasher into the new organization at any time, but a clean transfer would have several advantages:

Again, thanks for having created this great tool!

vog commented 5 years ago

@munjeni Thanks a lot for your trust and for moving this repository to the new organisation!

munjeni commented 5 years ago

You will need to update readme too?

vog commented 5 years ago

@munjeni If you refer to PR #17: Yes, I plan to update the README, too. But I believe the current README is still correct after #17, because the README doesn't say anything about what happens when "make" is called without parameters, and all "make" invocations mentioned in the current README still work ("make newflasher.exe", "make newflasher.x64", etc.)

munjeni commented 5 years ago

Sorry I see see now. To let you know do not wait me for revision, if you need to merge something just do it but please test it before you merge since whole newflasher is prety harcoded! I'm hope we get more contributors and revisitors here.

vog commented 5 years ago

@munjeni Okay, in case I'm running into time pressure, I'll merge pull requests on my own, and/or commit directly. Nevertheless, I value your review very much, even if it's just a superficial plausibility check ... of course, only as long as your time allows for that.

munjeni commented 4 years ago

@vog what is a progress of this?

vog commented 4 years ago

@munjeni Thanks for the reminder.

After the most important cleanups of the newsflasher code were finished, I had to switch priorities and (almost) finished my high-level tool Ait – Android installation tool which makes heavy use of newflasher when executed for Sony Xperia XA2 devices.

Now that Ait is in a pretty good state, it's time to go back to newsflasher and to finish the Debian package.

munjeni commented 4 years ago

Hi, almost 6 months gone, any progress?

munjeni commented 4 years ago

All tickets fixed and closed except this one, can you finaly make debian package?

vog commented 4 years ago

Will do. Sorry for the long delay. Meanwhile, I added a minor pull request.

vog commented 4 years ago

I added some final preparation steps, and confirmed that I can build a proper Debian package once those are applied.

It would be great if you could create a new version 30 based on those changes.

vog commented 4 years ago

Here is a preview of the manpage provided with pull request #32:

Alas, GitHub is unable to preview manpages on their own.

munjeni commented 4 years ago

Hi, thanks! I'm done some correction to man page https://github.com/newflasher/newflasher/commit/a73ff5582c9fb81798493a248582953768d9d004 and updated version to 30

vog commented 4 years ago

I just noticed that I need to add a custom "make dist" as well. The standard ".tar.gz" download by GitHub is insufficient, because it is too large as it contains that 4 MiB lib/ directory.

chirayudesai commented 4 years ago

Thanks for your work!

Tracking addition to Debian at https://salsa.debian.org/android-tools-team/admin/-/issues/23

vog commented 4 years ago

Ok, I just created a release tag "30":

And uploaded a relatively small (2.6 MiB) source package there (produced by "make dist").

munjeni commented 4 years ago

Great, thanks! I'm just checked responses from xda, seems everything is working great

munjeni commented 4 years ago

I think you need to exclude win32 stufs from https://github.com/newflasher/newflasher/commit/56cb1f6775314f983fd29309db27f12adf869aab , gordongate header , rc files, icon file... etc

vog commented 4 years ago

Thanks for the hint!

I'll fix that, and I propose to create a new release 31 to avoid confusion.

I also propose to clearly mark the "Optional step 1" as "Windows only", because instructions like "double click on the exe" would be confusing on Debian, especially when Newflasher is installed via package manager to a proper system location. Moreover, "Optional step 1" installs that flash driver that isn't needed for Debian at all (and wouldn't run there, anyway).

vog commented 4 years ago

Finished pull request.

If you create version 31, would you mind to create the corresponding Git tag "31" as well? I did that for releases 29 and 30, but I believe it should better be done by yourself, as part of your release routine.

At the Git command line, the whole release process would be:

EDIT files  # same as before
git commit ...  # same as before
git push # same as before
git tag 31
git push --tags

Note that in theory, you could leave out the third step, as the last command would push your commit as well as the tag. However, I recommend to first push your commit, just in case you were not up to date. You would then notice, fix your commit (e.g. merge or rebase) and finally tag the fixed commit.

munjeni commented 4 years ago

Thanks, ok I will try

munjeni commented 4 years ago

I'm done release 31, one problem trought this commit https://github.com/newflasher/newflasher/commit/5b08e9eb6d803277f833ed714c5d6de7dd70c959 is not seen between 30 & 31 , do you have idea why? I'm deleted first atempt for tag 31 and probably that the cause?

vog commented 4 years ago

Okay, I'll have a look at this.

chirayudesai commented 4 years ago

Hi,

In the tag '31' I also see the binary file newflasher-30.txz added to the repo. See https://github.com/newflasher/newflasher/commit/f866ad3a5eb99de4581dda4ac8d035ae7c38fcf7

Is that intentional? If not, maybe it should be added to .gitignore

vog commented 4 years ago

That's definitely a mistake from my side.

vog commented 4 years ago

I'll force-push a fixed commit, so this won't increase the history size.

munjeni commented 4 years ago

Oh sorry I'm done allready, Recreated tag 31 https://github.com/newflasher/newflasher/releases/tag/31 , still one commit -> https://github.com/newflasher/newflasher/commit/5b08e9eb6d803277f833ed714c5d6de7dd70c959 is missing between tag 30 and tag 31 -> https://github.com/newflasher/newflasher/compare/31...master , can it be github bug?

chirayudesai commented 4 years ago

Tag 30 shows as: https://github.com/newflasher/newflasher/commit/56cb1f6775314f983fd29309db27f12adf869aab Tag 31 shows as: https://github.com/newflasher/newflasher/commit/f866ad3a5eb99de4581dda4ac8d035ae7c38fcf7

'31' still has that newflasher tar xz.

If you go to https://github.com/newflasher/newflasher/commits/master , you will see the tag 30 commit, "Add "make dist" (#9)" However, the tag 31 commit is not seen, and even the tag says: "This commit does not belong to any branch on this repository. ", which is because of the force-push.

Solution: Recreate tag one last time, and this time it should be commit https://github.com/newflasher/newflasher/commit/4161eaa4426aa911ac715036442dba810d7aef02

Something like:

git tag -d 31
git show -1 # make sure this is showing commit 4161eaa4426aa911ac715036442dba810d7aef02 - change to version.h only
git tag 31
git push -f github 31

Also, if you have gpg keys set up, you can use git tag -s 31 to create signed tags.

munjeni commented 4 years ago

Sorry, using tags for the first time, solved with:

git reset --hard fa83521967f9e57af5d9fa18561a506929eef6c0 git push -f git tag -d 31 git push -f git push --delete origin 31

than done commits from beggining, than:

git tag 31 git push git push --tags

I'm unsure if what I done is right but the result seems ok now?

chirayudesai commented 4 years ago

@munjeni yes looks ok to me now.

https://github.com/newflasher/newflasher/releases/tag/31 is https://github.com/newflasher/newflasher/commit/29ab0839ac34e3c682c183dba7490fb29fec359c which is what I see at the top of https://github.com/newflasher/newflasher/commits/master right now.

munjeni commented 4 years ago

Ok thanks!

vog commented 4 years ago

@munjeni Looks fine to me as well. I also verified that your uploaded newflasher-31.txz is correct and contains all files in their correction version.

@chirayudesai I'll try to provide an answer to the question by @munjeni. Please correct me if I made some mistake.

why we need newflasher-xx.txz at all? Because of size of the archive? If it was a problem I can try to remove those libs and do similar change like this https://github.com/newflasher/newflasher/commit/3e15d1380b91d27fce928986e74e04ae7266ce30 for other cross blobs so it will not contain those prebuil libs.

It is partially about the archive size, but not in the sense that Debian wouldn't have those resources, it would just be a waste of those resources. We are talking about 4.9 MiB versus 24 KiB.

Moreover, there is the potential security danger that we, by accident, statically link to one of the provided pre-built libraries. This can't happen if those aren't in the source package in the first place.

And there is the potential licensing issue by distributing the GordonGate Windows installer binary (packaged into GordonGate.h). I didn't do any check here, I'm just noting that this is most easily solved by not having that file in the source package in the first place.

But again, @chirayudesai please correct me if I'm wrong.

munjeni commented 4 years ago

Ok, I agree, doing one more step (make dist) is not problem at all. In the time I will try to remove prebuilts and gordongate header and give link for download instead.

munjeni commented 4 years ago

I think it is much easier than need for make dist. Is this ok?

vog commented 4 years ago

This looks very good to me!

However, I think that the "cross_blob" is going too far. I propose to just unpack it back to the directory. There was nothing wrong with the ".rc", ".ico" and ".mk" files. I just excluded them from "make dist" because they are unneeded for Unix systems, which was perhaps not a good decision from my side in the first place.

munjeni commented 4 years ago

Ok, I'm done it. Should I make v32 ?

vog commented 4 years ago

Yes, please do so.

munjeni commented 4 years ago

It's done https://github.com/newflasher/newflasher/releases/tag/32 Looks better now :)

chirayudesai commented 4 years ago

@vog For Debian we can still just exclude anything we don't want in debian/copyright.

See https://salsa.debian.org/android-tools-team/newflasher/-/blob/3c94a06aae54db4bdc7585414ef0ce669920d2d2/debian/copyright#L5 (this was for version 30/31)

32 looks good, thanks for the update, will switch to that.

vog commented 4 years ago

This is a great progress! Thanks a lot to @munjeni and @chirayudesai for making this all possible.

@munjeni Just for clarity, you might have noticed that an important organizational change happened a few days ago. The future Debian maintainer for the Newflasher Debian package will not be myself, but instead the Debian Android Tools Team. This is great news, and this is why @chirayudesai is so involved here. It means that the fate of the Newflasher Debian package is not determined by a single person (like myself), but by a group of people who also maintains quite a lot of other Android development related tools for Debian.

munjeni commented 4 years ago

Thats great! New life for a newflasher! :)

munjeni commented 4 years ago

And thank you too for initiating all that!

munjeni commented 4 years ago

@chirayudesai can you please import version 36, its latest stable version and I'm believing will not need update for a long time

munjeni commented 3 years ago

Anybody mind to update Debian repository?

vog commented 3 years ago

@chirayudesai As far as I can see, there is no initial newflasher package yet in Debian, not even in Debian/unstable or Debian/experimental. Is there anything I can do to assist or support you with that?

Search for "newflasher" across all Debian versions (currently, returning an empty result):

munjeni commented 3 years ago

Hi, but what about this? -> https://salsa.debian.org/android-tools-team/newflasher

Its too old I see. I would be need to move back newflasher to my acoount (remove from organisation), can you do it?

munjeni commented 3 years ago

I'm done that and thats a final decision without need for explaining anything. If you want you could initiate newflasher removal from Debian, thank you! Closing this ticket.

vog commented 3 years ago

@munjeni As a final clarification, please note that the GitHub organization is in no way related to the Debian packaging. It's just a best practice, just like the many code improvements. Following such best practices makes it easier for distros to package newflasher, but is in no way mandatory.