Experimental Windows support - take 2 #172
Conversation
This protects against some users specifying Windows separators and some specifying Unix-like separators.
The CustomTask data class is defined as a child in this class so does not need qualification.
Add clarifying newlines.
A rule in case you have not exported the ANDROID_SDK_ROOT environment property.
Windows does not allow the : character to be used in paths.
...ledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleConfiguration.kt
Show resolved
Hide resolved
...ledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleConfiguration.kt
Show resolved
Hide resolved
...dmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetector.kt
Show resolved
Hide resolved
...etector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt
Show resolved
Hide resolved
...etector/src/test/kotlin/com/dropbox/affectedmoduledetector/AffectedModuleDetectorImplTest.kt
Show resolved
Hide resolved
| @@ -43,17 +43,18 @@ class AffectedModuleDetectorIntegrationTest { | |||
| @Test | |||
| fun `GIVEN multiple project WHEN plugin is applied THEN tasks has dependencies`() { | |||
| // GIVEN | |||
| tmpFolder.setupAndroidSdkLocation() | |||
There was a problem hiding this comment.
I am assuming that you all have an environment variable that points to your Android SDK location...... well I didn't 😝 . I don't think we should assume this in general hence this workaround.
There was a problem hiding this comment.
I slightly disagree here. We are working on an android project (AMD does kind of expect a proper android setup) and so one would assume that we've got an environment variable setup to point correctly
Actually creating a file on the users seems odd but I guess its in a temporary directory so that should be fine?
There was a problem hiding this comment.
AMD does kind of expect a proper android setup
This I agree with 💯.
and so one would assume that we've got an environment variable setup to point correctly
Well.... I'm not sure that gets set up by default and it is manual setup that will be needed to create this environment property. At least that's the conclusion I came to months ago when o was working on this on Windows. For example simply by installing the Android SDK I didn't have this environment property available. It might work in AS because theat sets up its own if I recall. So whilst I can agree that we would assume the Android SDK is available.... it doesn't necessarily have to be exposed via an environment variable.
I will try to set up a brand new project on Windows and Linux and see if they expose the property if you want?
Actually creating a file on the users seems odd but I guess its in a temporary directory so that should be fine?
Yeah I thought this was the least amount of work since it is transparent to users and it cleans up after itself. Hopefully that's cool 👍.
|
Hi @joshafeinberg apologies for the direct mention but would you mind giving this a look see? Happy to talk through in more detail if you have questions. |
Yeah sorry, things are pretty busy on my end but I promise to check this out by the end of next week at the latest :) |
| .split(File.separatorChar) | ||
| .toMutableList() | ||
| val projectRelativeDirectorySections = rootProjectDir | ||
| .toRelativeString(gitRootDir) |
There was a problem hiding this comment.
Can you explain how the projects root directory and the git root directory differ? Seems like they should be the same
There was a problem hiding this comment.
Can you explain how the projects root directory and the git root directory differ? Seems like they should be the same
Sorry I'm on mobile at the moment so can't back this up properly but.... If I remember correctly this was because the project I was working on had customised their Gradle project layout via settings.gradle.kts.
We had a structure such that the root and sub projects weren't structured like a "normal" Gradle project would be but instead nested deeply in file system (99% of the projects I have worked on follow the standard Gradle structure but we had to be different 😂). Gradle is cool with that since it uses settings.gradle.kts to resolve the File references to the root and sub projects but it breaks one of the assumptions that AMD uses.
Given that it was nonstandard but still valid Gradle I felt we should support those projects. Otherwise AMD reports false negatives.
So the git root directory would be, on the file system, the top most directory but the Gradle root project will be nested somewhere underneath it.
Sorry if that doesn't make sense I can draw a better diagram when I'm back?
There was a problem hiding this comment.
Yeah, diagram would be great.
I'm down for supporting weird setups but we have to kind of decide if its totally worth it for super weird setups ha. Otherwise this seems like a good use of the baseDir property that exists on our plugin (
There was a problem hiding this comment.
Sorry for the delay here's a diagram to explain what's in my head.
A "traditional" Gradle project will look like this:
my-app <-- projectRootDir & gitRootDir
|
-- .git
|
-- module-1
|
-- module-2
|
-- module-3
|
-- settings.gradle.kts
The .git folder is a child of the my-app folder on the file system and thus the my-app folder is the gitRootDir. There are no customisations in settings.gradle.kts other than to include the modules module-1, module-2, and module-3. Gradle will also see my-app as the projectRootDir. This is what most Android projects look like and is a fine and sensible layout to assume.
However I came across this setup at work:
android-project <-- gitRootDir
|
-- .git
|
-- diagrams
|
-- ci
|
-- proposals
|
-- my-app <-- projectRootDir
|
-- settings.gradle.kts
|
-- common
|
-- module-1
|
-- module-2
|
-- module-3
In this set up the gitRootDir and the rootProjectDir are not the same.
Having just typed all of that and also looking through the code again I think there is a simpler explanation. Whilst the above is still true.... there can be exotic projects..... the more simpler explanation is that I must have copied this over from the existing implementation. 😆
Also..... regarding baseDir. That is nullable (and thus optional) in the AMD Configuration so we can't rely on it being set unless we go for an API change - I am taking the view that the Configuration forms part of the API for this 🤷 .
There was a problem hiding this comment.
Yeah, to me this feels like a totally separate issue than windows support so is it possible to pull this logic related to git out? I see what you are saying but I think its best to continue this discussion in its own PR rather than continue to block this one if you agree
On the point of nullability, if a user doesn't configure their project correctly then we might not want to worry about internally handling all the edge cases :)
There was a problem hiding this comment.
Yeah, to me this feels like a totally separate issue than windows support so is it possible to pull this logic related to git out? I see what you are saying but I think its best to continue this discussion in its own PR rather than continue to block this one if you agree
Sorry I think I have sent us in the wrong direction here with talk about the alternative / exotic Gradle projects. I do totally agree that supporting these types of projects is out of scope of this PR. In fact in my original PR (#77) I included those additional changes but in this PR I have removed all but the Windows support since I agree the PRs should be focussed on one thing.
However with regards to Files.kt this is extracted from the code in ProjectGraph::findContainingProject and ProjectGraph::findProjectRelativeDir from the main branch. The reason I have done this is because I needed this functionality in two places: ProjectGraph and AffectedModuleDetector. Even in the main branch there is the use of gitRootDir and rootProjectDir. The modification I make is only to support Windows.
Again, massively sorry I gave the wrong impression in my first comment. This isn't to support exotic project set ups. I was planning on raising a PR for that in the future but it's not this one. I will raise a separate issue about the exotic projects and we can continue discussions there. 🙇
There was a problem hiding this comment.
Alright, sounds good. Thanks for the clarification and the work on this!
Codecov Report
@@ Coverage Diff @@
## main #172 +/- ##
============================================
+ Coverage 50.37% 51.11% +0.73%
+ Complexity 66 64 -2
============================================
Files 13 15 +2
Lines 532 540 +8
Branches 99 99
============================================
+ Hits 268 276 +8
Misses 237 237
Partials 27 27
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Attempt to address #37 by adding experimental Windows support (I say experimental because really what we should be doing is using an abstract file system like the one from Okio and write proper tests for each known target platform - Linux, Mac, Windows. However to begin with I just wanted to get something that worked on Windows.)
The main issue I noticed was that
GitClientImplseems to always return Unix-like line endings and path separators -"\n"/"/". However Gradle returns the appropriate constants for the target operating system. On Windows this is"\r\n"/"\". The AffectedModuleDetector (at its heart) compares the output from Git with the output from Gradle and compares the two to work out changes. Therefore each output needs to be in the same format.The main changes are:
":"on Windows 😢 ).This is a reworking of #77 done in my own time on my own laptop so that I can happily sign the CLA that took so long for my company to give me any sort of answer for 😅 . It also only adds Windows support and not the weird Gradle project structure support nor the build file changes as I realise the original PR was a bit messy in mixing all these concerns.