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

Improve target handling #11

Merged
merged 2 commits into from
Aug 20, 2019
Merged

Improve target handling #11

merged 2 commits into from
Aug 20, 2019

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Aug 5, 2019

This fixes #9 and #10.

The complexity is mainly in the fix for #10, to avoiding invoking rustc --version --verbose once per task. It completes somewhat quickly, but we often have a lot of tasks. Ideally we'd just use a Lazy, static ctor, or some other way of doing the same thing we do automatically, but our need to pass in the Project prevents this.

I also improved the documentation for targetDirectory in the fix for #10, since it didn't explain when you might want to use it, and i think the example could be confusing (since there's a release folder inside the target directory).

@thomcc thomcc requested a review from ncalexan August 5, 2019 23:08
Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these commits are basically good, but I expect that rewriting without project.exec and with lateinit or lazy will change stuff enough that I should look again.

Thanks for digging in, Thom!

plugin/src/main/kotlin/com/nishtahir/CargoBuildTask.kt Outdated Show resolved Hide resolved
plugin/src/main/kotlin/com/nishtahir/CargoBuildTask.kt Outdated Show resolved Hide resolved
plugin/src/main/kotlin/com/nishtahir/CargoBuildTask.kt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm basically happy with this, modulo the cargoCommand bit below. Add rustcCommand (and env vars) if it's not possible to get this data from cargo directly.

/** Attempt to get the default (desktop) target triple by parsing `rustc -Vv` output. */
private fun computeDefaultTargetTriple(): String? {
val runtime = Runtime.getRuntime()
// Do we need to worry about the env (specifically PATH) here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a cargoCommand and a pythonCommand. Can we determine this version from cargoCommand?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, we cannot. I'll add rustcCommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think doing this would prevent us from caching the output. Hmm, let me think if there's a problem with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, All the caching code is gone, we just invoke it every time based on the extension configuration. It's not fatal for us to fail to locate it, we'll just pass --target explicitly and warn.

@thomcc
Copy link
Contributor Author

thomcc commented Aug 19, 2019

I've cleaned up the commit history as well.

@thomcc thomcc merged commit 445abc6 into mozilla:master Aug 20, 2019
@thomcc thomcc deleted the target-handling branch August 20, 2019 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support CARGO_TARGET_DIR environment variable
2 participants