-
Notifications
You must be signed in to change notification settings - Fork 75
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
Guard against misconfigured target directory #47
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay before reviewing. I don't think this is right as written, but I'm happy to help the user do the right thing in this case.
@@ -42,6 +44,10 @@ open class CargoBuildTask : DefaultTask() { | |||
?: targetDirectory | |||
?: "${module!!}/target" | |||
|
|||
if (Files.exists(Paths.get(targetDirectory))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional looks backwards -- surely you want !Files.exists(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Thanks!
@@ -42,6 +44,10 @@ open class CargoBuildTask : DefaultTask() { | |||
?: targetDirectory | |||
?: "${module!!}/target" | |||
|
|||
if (Files.exists(Paths.get(targetDirectory))) { | |||
throw GradleException("Cargo target directory (`${targetDirectory}`) is not exists. If you're using Cargo Workspace, you need to specify the target directory in `build.gradle` or `local.properties` (see https://github.com/mozilla/rust-android-gradle#targetdirectory).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/is not exists/does not exist/. And s/Cargo Workspace/a Cargo workspace/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
When the Rust project is inside a Cargo Workspace this plugin will _fail_ silently because by default it assumes Cargo target directory will be inside the module directory. This patch add a guard against misconfigured target directory. When the target directory not exists, we'll make the build fails and point user to the documentation. ``` Cargo target directory (`../../target`) does not exists. If you're using a Cargo Workspace, you need to specify the target directory in `build.gradle` or `local.properties` (see https://github.com/mozilla/rust-android-gradle#targetdirectory). ```
When the Rust project is inside a Cargo Workspace this plugin will
fail silently because by default it assumes Cargo target directory
will be inside the module directory.
This patch add a guard against misconfigured target directory. When the
target directory not exists, we'll make the build fails and point user
to the documentation.