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

add Makefile #903

Closed
wants to merge 1 commit into from
Closed

Conversation

afcady
Copy link

@afcady afcady commented Jun 4, 2019

This allows the project to be built in a single command (make). The Makefile handles downloading and installing the SDKs in an essentially identical way to the .travis.yml, then builds with gradle.

However, builds with gradle are not fully incremental. They appear to be much more so today than a month ago. When I re-build with no changes, I get this:

BUILD SUCCESSFUL in 4m 12s
97 actionable tasks: 25 executed, 72 up-to-date

So 25 tasks were run unnecessary. Of course, 72 out of 97 ain't bad, but also 4 minutes ain't good. I was hoping I could get some assistance in making these builds properly incremental.

@jamorham
Copy link
Collaborator

jamorham commented Aug 3, 2019

I would be surprised if you can get incremental builds working in any real sense due to the post processing done by proguard.

@afcady
Copy link
Author

afcady commented Aug 3, 2019

Is it easy enough just to disable proguard conditionally?

@tolot27 tolot27 added the code:quality code and repository related label Dec 7, 2020
Copy link
Collaborator

@tolot27 tolot27 left a comment

Choose a reason for hiding this comment

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

Version numbers and file names are hard-coded. Can you fetch them from the gradle build scripts?
Also, it is assumed that gradle is installed. Maybe, you should fetch it based on the distributionUrl in gradle-wrapper.properties.

Incremental compilation is mostly handled by Gradle or the Android Gradle plugin and also depends on preprocessers like annotators. Newer versions of gradle improve this. See also #1522 for further information.

@tolot27
Copy link
Collaborator

tolot27 commented Dec 7, 2020

Is it easy enough just to disable proguard conditionally?

Then you will loose the benefits of ProGuard. It is better to upgrade to Android Gradle plugin 3.4.0 which uses R8 to achieve the same tasks ProGuard addresses.

@afcady
Copy link
Author

afcady commented Jan 10, 2021

Yeah, should be easy to do that. Gonna look at this again soon.

@tolot27
Copy link
Collaborator

tolot27 commented Mar 15, 2021

@afcady Please add the requested changes or delete this PR if you don't need it anymore. If you have questions, feel free to ask.

@Navid200
Copy link
Collaborator

@afcady Would you please update this with your plans?
Are you still interested?
Or, can we close this pull request?

Thanks

@Navid200
Copy link
Collaborator

Can we close this pull request please?
We can always reopen it if the developer likes to work on it again.

@tolot27
Copy link
Collaborator

tolot27 commented May 12, 2021

It looks like this feature is not required anymore. Closing due to inactivity.

@tolot27 tolot27 closed this May 12, 2021
@afcady
Copy link
Author

afcady commented May 18, 2021

I don't know why you don't just merge the file, but I will go back and add the feature requested eventually.

@afcady
Copy link
Author

afcady commented May 18, 2021

It's actually no big deal to update the file when the dependencies change, so maybe that feature is silly. Anyway, it is no reason to block the merge or to close the request.

@tolot27
Copy link
Collaborator

tolot27 commented May 19, 2021

@afcady You did not respond for a long time and we don't like to add files which we have to maintain because they contain fixed strings instead of dynamically changed variables. Currently, you are the only user which needs/likes this feature. If you like to get it merged into xDrip, please follow our suggestions, especially, if it is easy to implement it by you. Otherwise, it is unlikely that this PR will ever be merged.

@afcady
Copy link
Author

afcady commented May 20, 2021

Maybe you should just merge the Makefile and then remove it if/when the fixed string ever stops working. That is way more normal.

I can write a Makefile that can try to parse these values out of the rest of the source code. Then we can hope that the rest of the source code won't break that crazy ouroborous code. It makes more sense though, to just allow the fact, that every few years when you change library versions, you have to rely on someone updating those values.

It is easy to keep the fixed strings updated just by accepting of patches!

But, some day, I will write the crazy ouroborous code, maybe. Keeping in mind, I don't use xDrip and these was just preliminary work to try to get to the point where I can work on the code enough to get it to work for me.

@afcady
Copy link
Author

afcady commented May 20, 2021

Did any of the fixed strings actually change since Jun 4, 2019?

I started writing some code like this:

+BUILD_TOOLS_VERSION := $(shell sed -n 's/^ *- build-tools-//p' .travis.yml)
+
 GIT_DISCOVERY_ACROSS_FILESYSTEM=y
 export GIT_DISCOVERY_ACROSS_FILESYSTEM

-SDKS = "platforms;android-26" "build-tools;27.0.3" "extras;google;m2repository"
+SDKS = "platforms;android-26" "build-tools;$(BUILD_TOOLS_VERSION)" "extras;google;m2repository"

...and then it occurred to me. This value has to match the sdk-tools-linux-4333796.zip file. Future sdk-tools-linux-*.zip releases by Google will have unpredictable URLs. So you really cannot make this work without ever having to change any strings in the future. A feature that literally no one wants, not even you, since you don't even want the file!

Anyway I can go ahead and implement a feature that cannot ever work, and that no one wants, since you insist on a feature no one wants before you will accept a feature only one person wants!

@afcady
Copy link
Author

afcady commented May 20, 2021

Answering my own question with git blame origin/master .travis.yml:

54e30f2 (Martin Minka 2020-01-10 08:53:18 +0100 23) - build-tools-28.0.3

So this file value change, but the last time was over a year ago.

I really think that updating this string does not need to be automated. But, it's your project, and you insist on having this feature, that you also tell me in the same breath, you do not want and will not use.

@tolot27
Copy link
Collaborator

tolot27 commented May 20, 2021

Keeping in mind, I don't use xDrip and these was just preliminary work to try to get to the point where I can work on the code enough to get it to work for me.

If you don't use xDrip, what is then your intention to contribute to it?

Our main goal (beside fixing bugs) is to reduce the complexity of xDrip and increase the maintainability. If we accept new code we request for unit tests which run automatically by CI and on every PR. This PR does not provide a test case, yet. If it would, it will likely fail in the future if we change the various API levels. Then we have to maintain a file which is not used by everyone, except you.

I suggest, you keep the make file in your master branch and use it on your own. There is no need to integrate it into xDrip master because xDrip builds fine and your on branches will build, too.

In my fork and local copy there are several files I'll never track, commit or push. That's not a problem.

Maybe, I've overseen the real benefit/advantage of you PR for the xDrip users or developer community. In that case, please explain it again.

afcady pushed a commit to afcady/xDrip that referenced this pull request May 21, 2021
@afcady
Copy link
Author

afcady commented May 21, 2021

@totol27 there is no need for discussion. I have implemented the parsing so that the fixed strings are gone.

@afcady afcady mentioned this pull request May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code:quality code and repository related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build troubles. (xDrip should have a reliable documented automated build procedure)
4 participants