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 #1735

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

add Makefile #1735

wants to merge 8 commits into from

Conversation

afcady
Copy link

@afcady afcady commented May 21, 2021

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.

This is an updated version of PR #903 which incorporates the requested change by @tolot27 (obtain version strings from .travis.yml) and uses a new .zip for sdkmanager.

Also related #768.

@Navid200
Copy link
Collaborator

It's more efficient if you work with the person who has reviewed your PR instead of opening another PR.
Is there a reason you couldn't use the same PR to make the necessary changes?

@afcady
Copy link
Author

afcady commented May 21, 2021

I am absolutely working with @tolot27: I wrote this code per their request!

I made a new PR because the new commits that I made didn't show up in Github UI on the other PR.

I guess that was because the PR was closed.

@Navid200
Copy link
Collaborator

This was in his last message:

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.

Which means, he is suggesting we don't need this.

@afcady
Copy link
Author

afcady commented May 21, 2021

There is no need to remind me of that.

Nevertheless, I made the requested changes.

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.

Thanks for addressing my requests. It looks much better than before. I still have some suggestions.

Since you have claimed not using xDrip, what is your intention to open this PR? Do you develop some automated deployment pipelines for xDrip or do you plan to address existing issues?

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@tolot27 tolot27 added the code:quality code and repository related label May 22, 2021
@afcady
Copy link
Author

afcady commented May 22, 2021

what is your intention to open this PR?

To share the code that I wrote to build xDrip.

Can I ask you a question: what is your intention to ask me all of these questions? Why do you care about the "code style" of the Makefile? I made the changes you requested.

I wrote the entire file and it is in my typical style. It is the only file in that language in the repository. So it should be in my style, as the sole author in that language in the project. In my style, variables that hold the names of commands are lowercase for consistency with other commands.

@afcady
Copy link
Author

afcady commented May 22, 2021

  1. The stamp files are good. That's the best way to do these things. Forget about it Jake, it's Unix.

  2. Using the package manager requires root. Also, supporting all the various package management systems is a lot of work. Many hours of setup just to do any kind of testing. If it is really important, better to merge first, then create a new issue for such complicated feature.

@15characterlimi
Copy link

15characterlimi commented May 25, 2021

My personal view is that this PR adds negative value and should not be merged.

The reason it adds negative value is that it duplicates some assumptions such as Android SDK version with which the project can be build, and gradle target names. Every time those change in future, one would then have to make a corresponding change in the makefile, or else the makefile would undergo bitrot.

Since we already have a build system (gradle) and since not even the author of this PR needs this since they don't use xDrip, this seems to not solve any problem that anyone has, while creating new problems as per my previous paragraph.

@tolot27
Copy link
Collaborator

tolot27 commented May 25, 2021

My personal view is that this PR adds negative value and should not be merged.

I would not call it a negative value as long as it has no negative impact.

@tolot27
Copy link
Collaborator

tolot27 commented May 25, 2021

what is your intention to open this PR?

To share the code that I wrote to build xDrip.

But what are the benefits for us? We already have an established build system and if we like to build it using Linux, I would simply install travis and using the existing .travis.yml file.

Can I ask you a question: what is your intention to ask me all of these questions?

Because we appreciate contributions but have to identify the benefits for us and the impact of a PR rejection.

Why do you care about the "code style" of the Makefile? I made the changes you requested.

I wrote the entire file and it is in my typical style. It is the only file in that language in the repository. So it should be in my style, as the sole author in that language in the project. In my style, variables that hold the names of commands are lowercase for consistency with other commands.

You are free to use your code style in your fork. But we like to have a common code style in the main repository. That are our rules and we recommend to follow common code styles and apply code formatting.

@15characterlimi
Copy link

The negative impact that I was referring to was the fact that it adds duplication of the names of gradle build targets and version numbers of Android SDK and build tools, which already exist in gradle files. This only adds a little bit of work towards changing these (because now the makefile needs to be changed and tested as well), but it sounded like no one is actually using the makefile, not even the original author?

Note that I don't feel strongly. There are much, much bigger problems in this project's code health than the potential duplication added by this PR.

@afcady
Copy link
Author

afcady commented May 25, 2021

The makefile allows a new user to build the project with:

  1. gigabytes less of dependencies
  2. fewer steps
  3. less reading of documentation
  4. less potential for human error

Building the project is the first thing users do BEFORE they write any code and become contributors.

Furthermore, without any Makefile at all, the make command does not work at all.

Even a non-working Makefile is not harming anyone. It is just there, ready to be fixed an updated like any other part of the code.

But I'm not being asked to address a non-working Makefile. Instead, I'm being asked to go read a style guide that I HEAVILY SUSPECT does not include any references to the Make language.

@afcady
Copy link
Author

afcady commented May 25, 2021

The reason it adds negative value is that it duplicates some assumptions such as Android SDK version with which the project can be build, and gradle target names. Every time those change in future, one would then have to make a corresponding change in the makefile, or else the makefile would undergo bitrot.

How is the non-working makefile of less value than a non-existent makefile?

Suppose we assign the values like this:

Makefile works | Makefile does not work | Makefile does not exist
  1            |  x                     |  0               

Why is x negative? I believe that x is positive and very close to 1 because the makefile in that condition can be more easily updated to work than in the non-existent condition.

@afcady
Copy link
Author

afcady commented May 25, 2021

I totally accept a 0 for the sake of argument though. But negative? How???

@afcady
Copy link
Author

afcady commented May 25, 2021

Let me just throw on another advantage. If a developer wanted to do automated builds for tests but not on Microsoft servers, the Makefile would enable that. It reduces dependency on Github and Microsoft testing servers.

You've got code that's incomplete, non-portable, runs only on one system, Travis. Now I add code that does the same thing but works on MY system, and is sure to work on MORE systems than the build system you guys had. Somehow my more portable solution is the problem, and especially the casing of its variables? Maybe it is a problem in YOUR mind. Syndrome called NIHS?

@afcady
Copy link
Author

afcady commented May 25, 2021

I got xdrip to build on a system it could not build on until I added those capabilities.

Why? That was the system I was using.

Why submit it here? Because it is more useful to have a more portable build system. Not just to me. More portable is objectively better.

Many other people have systems that will not run . I share my work with them for their benefit. You people here stand in the way of me sharing my free work with other users. To no benefit to you or me or anyone whatsoever to block my makefile nor to have any of these arguments. What are you trying to do? Gatekeep? Drive me away? This is not helping xdrip or anyone else in the world. Unlike the makefile. The makefile can help someone with a system like mine accomplish the build that I accomplished but withot performing the work I performed.

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.

4 participants