nmeum / android-tools

Unoffical CMake-based build system for android command line utilities
Apache License 2.0
177 stars 51 forks source link

CMake: Add option to patch vendor projects #87

Closed Biswa96 closed 1 year ago

Biswa96 commented 1 year ago

Set it to ON by-default to be compatible with previous choice. This helps to start with clean vendor directories.

anatol commented 1 year ago

Could you please add more information about where this option is useful? What exactly are you trying to achieve with it?

Biswa96 commented 1 year ago

I want to compile the project without patching any project in vendor and modifying any file in vendor. This option helps to toggle the patching procedure from command line.

This also helps me to upstream changes. For example, I patch every projects in vendor except boringssl. Then compile and fix boringssl according to the error output. If the cmake option is not present cmake always try to clean and patch submodules. So, the option also helps to retain any changes in submodules.

Biswa96 commented 1 year ago

Feel free to change the variable name or help text in option cmake function. I am not good at naming things :)

anatol commented 1 year ago

Is this feature really need to be in the mainline? Does anybody else need this?

How much trouble for your development would be if you just keep it in your local repo?

Biswa96 commented 1 year ago

Is this feature really need to be in the mainline?

This feature helped me, so I thought it may help others also.

Does anybody else need this?

I did not discuss about this with others.

How much trouble for your development would be if you just keep it in your local repo?

No trouble. Besides, why do developers care about sharing changes and ideas with each other? Don't they just keep that in local repository? :)

anatol commented 1 year ago

Sharing ideas and patches is really good. It is what github is for.

Just to be clear on what I am trying to say here. If this change is useful for others then I am for merging it. But I need more input from other developers/users. cc @nmeum, what do you think of the patch?

Otherwise, we might keep this PR open until more people need it and then merge the change.

Biswa96 commented 1 year ago

Sure, I shall wait for further review.

nmeum commented 1 year ago

cc @nmeum, what do you think of the patch?

I don't have a strong opinion on this. I don't think this is a common use case but the changes are quite minimal so I also wouldn't mind having this in the main branch. However, if you add to the main branch than please document the purpose in the commit message and maybe also elaborate a bit on the option with a source code comment in the CMake configuration file.

Biswa96 commented 1 year ago

Thank you for the review. This option helped me to detect upstream and duplicated patches. I have added comments in git commit and in cmake file. Please let me know if I have to add or change anything.

anatol commented 1 year ago

Okay, let's move forward with this patch, then.

Biswa96 commented 1 year ago

Can you enable the discussion tab please? I would like to discuss some casual topics before working on that.

nmeum commented 1 year ago

Sure, done.