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

Change app id based on current git branch #3133

Merged
merged 5 commits into from
Mar 11, 2020

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Feb 23, 2020

As stated in #3114 (comment) I changed the gradle script so that it adds the current branch name to the application id and to the app name.

Since the application id cannot contain special characters, I remove all non-letter characters from the branch name when appending it to the app id. When the current branch is "master" or "dev", everything is just like before (i.e. the app name is "NewPipe Debug" and the app id is "org.schabi.newpipe.debug"). When building a release apk everything is like before, too (i.e. the app name is "NewPipe" and the app id is "org.schabi.newpipe").
I removed the debug AndroidManifest.xml because its only purpose was that of setting the app name to "Android Debug" in debug builds, but this is now done by the gradle script.

Thanks to this change we would be able to install multiple builds from different branches at once.

For example take this PR: the branch name is gradle-app-id-suffix. Therefore the app name is "NewPipe gradle-app-id-suffix" and the app id is "org.schabi.newpipe.debug.gradleappidsuffix".

This enables to install multiple builds from different branches at once
@Stypox Stypox added the meta Related to the project but not strictly to code label Feb 23, 2020
@B0pol
Copy link
Member

B0pol commented Feb 23, 2020

Since the application id cannot contain special characters, I remove all non-letter characters from the branch name when appending it to the app id.

Why not do like #2309 , NewPipe PR#NUMBER, or just NewPipe #NUMBER?

same for package suffix, we could end up with having not unique package names (but maybe it's rare and not a problem, I don't know)

Otherwise I confirm it worked for me

@wb9688
Copy link
Contributor

wb9688 commented Feb 23, 2020

@B0pol: Because you can't know what PR number it will be in Gradle.

@Stypox
Copy link
Member Author

Stypox commented Feb 23, 2020

@B0pol There is no way to get the pr number: it would require making requests to GitHub to check if there is an open pull request from that branch and if that's the case get the number. There are a few issues: I'm not even sure if this can be done; connecting to the internet just to build an app is not a good idea, imo; what if the user wants to build the app before having created a pr?
Yeah, we could end up with conflicts in package names, but they would be rare and this solution would be better that always having conflicts anyway

Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

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

question

app/build.gradle Outdated
Comment on lines 33 to 36
def workingBranch = "git rev-parse --abbrev-ref HEAD".execute().text.trim()
if (workingBranch.isEmpty() || workingBranch == "master" || workingBranch == "dev") {
applicationIdSuffix ".debug"
resValue "string", "app_name", "NewPipe Debug"
Copy link
Member

@B0pol B0pol Feb 23, 2020

Choose a reason for hiding this comment

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

Uh, won't it add .debug and " Debug" to master? Because if I'm wide awake, with || workingBranch == "master" master is true on this if block, and has .debug suffix, and NewPipe Debug as name.

Have you tried to apply your changes in the master branch and launch the app, to see what happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

The branch name is independent from the build type. So we can have a release build on any branch and a debug build on the master branch. Gradle has no idea whether the current branch is a development branch or not, so it can't do decisions based on that.
So it is ok for a debug build on master branch to have .debug at the end

@B0pol
Copy link
Member

B0pol commented Feb 23, 2020

@B0pol There is no way to get the pr number: it would require making requests to GitHub to check if there is an open pull request from that branch and if that's the case get the number. There are a few issues: I'm not even sure if this can be done; connecting to the internet just to build an app is not a good idea, imo; what if the user wants to build the app before having created a pr?
Yeah, we could end up with conflicts in package names, but they would be rare and this solution would be better that always having conflicts anyway

You're right, I didn't think about you need to upload first to get a pr number.

@connectety
Copy link
Contributor

Someone may not name their branches and just code on their dev branch.
What about adding the username to differentiate between branches with the same name?
To make sure the name is not to long, maybe just do it if the branch is dev or master and the user is not TeamNewpipe.

NOT YET TESTED:
You can get just the username with:

def remoteName = "git remote".execute().text.trim()
def remoteURL = ("git ls-remote --get-url" + remoteName).execute().text.trim()

def pattern = ~/^([^@\n]+@|https:\/\/).*[:\/)](.+)\/.+\.git$/
def matcher = pattern.matcher(remoteURL)
def username = matcher.group(2)

You can get the remote username and branch with (this runs one less command and may be faster):

def remoteName = "git remote".execute().text.trim()
def remoteURL = ("git ls-remote --get-url" + remoteName).execute().text.trim()

def pattern = ~/^([^@\n]+@|https:\/\/).*[:\/)](.+)\/(.+)\.git$/
def matcher = pattern.matcher(remoteURL)
def username = matcher.group(2)
def branch = matcher.group(3)

@wb9688
Copy link
Contributor

wb9688 commented Feb 23, 2020

@connectety: That's IMHO a bad idea and could break more easily, e.g. that .git at the end is not required, SSH instead of HTTPS cloning is also possible, etc

@connectety
Copy link
Contributor

connectety commented Feb 23, 2020

@wb9688 just throwing around ideas.
the regex should not break thought, i tested with:

https://github.com/TeamNewPipe/NewPipe.git
[email protected]:TeamNewPipe/NewPipe.git
https://connectety:[email protected]/TeamNewPipe/NewPipe.git
[email protected]:connectety/NewPipeStub.git
https://gitlab.com/connectety/NewPipeStub.git
https://oauth2:[email protected]/connectety/NewPipeStub.git

@wb9688
Copy link
Contributor

wb9688 commented Feb 23, 2020

@connectety: What about https://github.com/TeamNewPipe/NewPipe or ssh://[email protected]:22/TeamNewPipe/NewPipe.git or http://localhost:12345/NewPipe.git or file:///some/path/to/NewPipe.git?

@connectety
Copy link
Contributor

connectety commented Feb 23, 2020

@wb9688 This is just a PoC to show that the idea of adding the username could be implemented. This was thought to enable a discourse about for example: build time. Its not about fixing all the details.

If the Author thinks, this is a good idea & I can find the time & the current solution is not used. I might write something more stable using the URI and URL classes in groovy (maybe separate PR). (this would increase the build time more). I'd prefer to just fix the most common errors in regex, but not all.

That being said, I don't think this should cover all edge-cases, however unlikely they are. The current implementation in this PR for example doesn't cover a good amount of edge cases like:

  • A branch name which only consists of Chinese characters
  • A branch name which is an issue number
  • Git beeing installed, but not beeing in the path
  • Downloading the repo via the github download button and not having set up git on their PC

For now this is just a PoC to get an opinion from the Author of the PR @Stypox.
I'm also fine with just dropping the idea.

@Stypox
Copy link
Member Author

Stypox commented Feb 25, 2020

I don't think this should cover all edge-cases

I agree, the purpose of this pr is to have different apk names most of the times, but it is not something we can't live without. So I think a simple implementation that works most of the times is enough, in case of errors it just falls back to the old .debug suffix. (I'm going to add more checks though, as this could fail with strange branch names)

@wb9688
Copy link
Contributor

wb9688 commented Feb 25, 2020

Imho it should just use the branch name like @Stypox already implemented.

- Add function `getGitWorkingBranch` that returns the current working branch, and "" if it could not be determined (either because git is not installed or because the directory is not a git repo).
- Make sure normalizedWorkingBranch is not empty (leading to an invalid app id terminating with `.`)
- Make normalizedWorkingBranch lowercase
- Add comments
@Stypox
Copy link
Member Author

Stypox commented Mar 2, 2020

I fixed the issues that could have caused a build failure:

  • Add function getGitWorkingBranch that returns the current working branch, and "" if it could not be determined (either because git is not installed or because the directory is not a git repo).
  • Make sure normalizedWorkingBranch is not empty (leading to an invalid app id terminating with .)
  • Make normalizedWorkingBranch lowercase

The code that adds the suffix should now be safe: in case of problems it defaults to the old .debug and NewPipe Debug values. So this PR is now ready.

@avently
Copy link
Contributor

avently commented Mar 10, 2020

@Stypox this is cool PR. So useful for everyone

@TobiGr TobiGr merged commit f7822a4 into TeamNewPipe:dev Mar 11, 2020
@B0pol
Copy link
Member

B0pol commented Mar 13, 2020

I feel like there is something important missing. Try to share a YouTube link, you'll see they all are newpipe, except NewPipe PR#2309 if you've installed it.
Could you please take a look or ask @mauriciocolli and change that?

@B0pol
Copy link
Member

B0pol commented Mar 13, 2020

I found another problem. For me, android studio is launching NewPipe Debug version when I click on run, instead of NewPipe branchname. It still install the NewPipe branchname app, but doesn't launch it.

@mauriciocolli
Copy link
Contributor

@B0pol

Try to share a YouTube link, you'll see they all are newpipe, except NewPipe PR#2309 if you've installed it.

Fixed in #3225.

For me, android studio is launching NewPipe Debug version when I click on run, instead of NewPipe branchname. It still install the NewPipe branchname app, but doesn't launch it.

I noticed that changing branches with Android Studio (doesn't matter if you change externally) almost always break something, whether is the "launch default activity" (your case) or even the code that is builds from.

Only solution is, with luck, forcing a sync using the options in the "File" menu or, when that doesn't work, restarting the whole IDE.

Surely an area that it could improve on.

@Stypox Stypox deleted the gradle-app-id-suffix branch March 14, 2020 09:03
@opusforlife2
Copy link
Collaborator

@Stypox Could you also automatically rename the debug apk that is generated?

@TobiGr
Copy link
Contributor

TobiGr commented Mar 25, 2020

According to the master of all questions, using project.archivesBaseName should work.

Will do a PR in a minute

This was referenced Mar 25, 2020
@opusforlife2
Copy link
Collaborator

... This might not have been such a good idea. I now have eight of them installed. 😅

@B0pol
Copy link
Member

B0pol commented Mar 25, 2020

There is a button called uninstall, it's really useful /s

@opusforlife2
Copy link
Collaborator

One guy to install them all,
debug apks to try them,
one guy to look for bugs,
and in his testing find them.

@TobiGr
Copy link
Contributor

TobiGr commented Mar 25, 2020

... This might not have been such a good idea. I now have eight of them installed. 😅

@opusforlife2 An easy way to remove those at once (This feature was added by some lazy devs at Google IIRC):
Go to settings > Backup & reset > Factory reset 🤣

@opusforlife2
Copy link
Collaborator

Instructions unclear. Phone exploded. Eyebrows gone. 🤷‍♂

@TacoTheDank
Copy link
Member

@opusforlife2 Geez, I didn't know the Note 7 was THAT sensitive 😳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to the project but not strictly to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants