search

FIRST-Tech-Challenge / FtcRobotController

BSD 3-Clause Clear License
686 stars 3.98k forks source link

Suggested changes to build.common.gradle #318

Open alan412 opened 2 years ago

alan412 commented 2 years ago
Windwoes commented 2 years ago

Regarding ndkVersion, while it is true that having that line is not required for a successful build, running a build with whatever version of the NDK happens to be installed means that builds may not be binary-reproducible which is not ideal. If a specific NDK version that someone is using happens to cause an issue, it would be a nightmare to track it down.

As for arm64-v8a, there do now exist Android devices with ARM cores that cannot run 32-bit code - so we are hesitant to drop 64-bit by default. The annoyance of having to remove that line for EasyOpenCV is something that could be resolved fairly easily in a future version of the library; I'd recommend making an issue on its GitHub page :)

alan412 commented 2 years ago

For the ndkVersion: This is no different than not requiring a specific SDK version which is not required. (It requires only a minimum one) Or even having things be different based off of whether it is compiled on different versions of Android Studio. Right now there is nothing that guarantees that two things built using the same FTC SDK will generate the same binary. Is there another reason I am missing why ndkVersion is a bigger deal than these (or perhaps other) things?

For arm64-v8a: It looks like there already is issue #39 on the EasyOpenCV package (which you have responded to)

Thanks,

Alan

On Fri, Apr 29, 2022 at 5:53 PM Windwoes @.***> wrote:

Regarding ndkVersion, while it is true that having that line is not required for a successful build, running a build with whatever version of the NDK happens to be installed means that builds may not be binary-reproducible which is not ideal. If a specific NDK version that someone is using happens to cause an issue, it would be a nightmare to track it down.

As for arm64-v8a, there do now exist Android devices with ARM cores that cannot run 32-bit code - so we are hesitant to drop 64-bit by default. The annoyance of having to remove that line for EasyOpenCV is something that could be resolved fairly easily in a future version of the library; I'd recommend making an issue on its GitHub page :)

— Reply to this email directly, view it on GitHub https://github.com/FIRST-Tech-Challenge/FtcRobotController/issues/318#issuecomment-1113783102, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF3Y77GJUTVOADZIOQA2ALVHRK67ANCNFSM5UV6J7ZA . You are receiving this because you authored the thread.Message ID: @.***>

Windwoes commented 2 years ago

For the ndkVersion: This is no different than not requiring a specific SDK version which is not required. (It requires only a minimum one) Or even having things be different based off of whether it is compiled on different versions of Android Studio. Right now there is nothing that guarantees that two things built using the same FTC SDK will generate the same binary. Is there another reason I am missing why ndkVersion is a bigger deal than these (or perhaps other) things?

Even with different Android Studio versions, though, the gradle files still specify specific versions of the Android SDK and Android Gradle Plugin etc. to build against. The native build also tends to be a bit more fragile I think; I do recall that the original library our UVC driver is based on was very finicky about which NDK version was being used for the build. Not sure how much of that does or doesn't apply to the current state of our UVC driver.... but having some degree of control over the build tooling used is better than none at all.

alan412 commented 2 years ago

The problem with the current specification of NDK is that it will not build with CI (either Travis or GitHub actions.). We use CI where students can only submit to a private branch and it has to build before they can merge into master. (I think the problem is that the NDK specified doesn't match one of the ones in the SDK specified)

I am happy to work with you to come up with a suggestion that satisfies both your desire to specify a version and come up with something that will build with CI. If I come up with a specific version that works, are you willing to change it in the build.common.gradle?

Windwoes commented 2 years ago

The issue here seems to be that the version of the NDK supported by GitHub Actions is a moving target. We did experiment with using GitHub actions internally some time ago, and looking at the description on the internal PR, we had to specify a specific version because the container didn't support the default version used by the gradle plugin: Capture

Then fast forward a little while and GitHub broke it again:

Capture Capture

Lunerwalker2 commented 2 years ago

As a user of such a workflow that builds on each push myself, I can attest that it is annoying when Github Actions seems to randomly change its expected versions every so often. I usually just do one commit to trigger the workflow, wait until it fails, find the error line that says what versions are available locally, and then choose one and add something like ndkVersion '22.1.7171670' to my build.common.gradle file (which is the current ndk version that is working for me). I've had to repeat this process for each new repository I've made in the past few seasons.

That being said, it seems to only switch every few months to a year, and it only takes about 10 minutes to fix the workflow when it does. This season for example only required one such fix at kickoff for me.

alan412 commented 1 year ago

Hmmm. Things change over time. I can tell you that right now, it works for us without an NDK specified. (You can see our repo here: https://github.com/ftc16072/2022Preseason - These are the changes we make to build.common.gradle: https://github.com/ftc16072/2022Preseason/commit/4563ae65181965243b8706ac14846dc6762d0fab ).

If I look back at commit history for FreightFrenzy, it looks like we changed minSdkVersion so Travis would pass. (We used to use TravisCI before they started charging)

So perhaps it requires a certain minSdkVersion to not require ndkVersion. That seems strange, but it wouldn't be the strangest thing I have seen....

--Alan

On Sat, Apr 30, 2022 at 11:34 AM Windwoes @.***> wrote:

The issue here seems to be that the version of the NDK supported by GitHub Actions is a moving target. We did experiment with using GitHub actions internally some time ago, and looking at the description on the internal PR, we had to specify a specific version because the container didn't support the default version used by the gradle plugin: [image: Capture] https://user-images.githubusercontent.com/26417650/166111931-69100fe0-701d-4492-8f0e-dd4937563730.JPG

Then fast forward a little while and GitHub broke it again:

[image: Capture] https://user-images.githubusercontent.com/26417650/166112025-2c813dcc-bdde-42c8-8400-74e36f5bc3e8.PNG [image: Capture] https://user-images.githubusercontent.com/26417650/166112111-18fdefe9-30bd-4aa0-afee-7accb0cb983b.PNG

— Reply to this email directly, view it on GitHub https://github.com/FIRST-Tech-Challenge/FtcRobotController/issues/318#issuecomment-1114007317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF3Y76PLFYD4GA42YWF2NLVHVHHDANCNFSM5UV6J7ZA . You are receiving this because you authored the thread.Message ID: @.***>