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

Optional outputs #291

Merged
merged 5 commits into from
Mar 2, 2020
Merged

Conversation

restingbull
Copy link
Collaborator

Changes the builder to be more intelligent about work execution: If an output is not requested, it is not created

Jdeps output is now optional, as it is not currently consumed.

Fix for java_test runfiles and kt_jvm_import. native rules consume runfiles in a separate private provider, to ensure that all starlark runfiles are picked up, each starlark rule should create runfiles.

Depends on: //pull/290

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@restingbull
Copy link
Collaborator Author

@googlebot I consent.

Gotta figure why this fails the check. One of these days.

@cgruber
Copy link
Collaborator

cgruber commented Feb 27, 2020

I'll give this a review once it's synced.

Also, I think the CLAbot fails because your PR includes (because of hte way you merged master into this branch, rather than rebasing on top of master) it includes one of my commits. Because there are commits from authors that aren't the PR author, they need to consent. So it's actually my commit that's at issue. If you rebase on git fetch upstream && git rebase -i upstream/master and clean out all the other commits, I think it won't be necessary to get that consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Corbin Smith added 3 commits February 28, 2020 17:19
…ested do not create it.

Break apart compileAll to allow stage independent compilations.
Make jdeps optional, as it is not currently consumed so we shouldn't do the work.
Update test framework to handle the output scheme.
If kotlin srcs are absent, skip kotlin compilation.

Issue: bazelbuild/issues/232
…e explicit and resilient that using collect_default.
Copy link
Collaborator

@cgruber cgruber left a comment

Choose a reason for hiding this comment

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

Approved with some nits and comments.

val friendPaths = info.friendPathsList.map { Paths.get(it).toAbsolutePath() }
val cp = inputs.joinedClasspath
val args = mutableListOf<String>()
args.addAll(baseArgs() + listOf(
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • isn't lazy, here, unless you're passing Sequence. While this is hardly the place where key optimizations need to happen, this will create a new list, and add all the lists, only to add them to a list. 🤷‍♂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not worry about it, but this was just to remind you about + and lists.

Copy link
Collaborator Author

@restingbull restingbull Mar 2, 2020

Choose a reason for hiding this comment

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

Hrm. Kotlin isn't smart, I guess. Builder time. Mostly, I didn't like the mutable list passing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, it's not that big a deal (especially at this N) - you can just use sequences here, and it'll do the same thing. Just remember, Lists - eager, Sequences - lazy, and you're fine.

return args
}

internal fun JvmCompilationTask.baseArgs(): List<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please pick a style. getFoo() or foo() (or internal val JvmCompilationTask.commonArgs get() = if you want to be all kotlin-properties about it)

(as in, getCommonArgs() is a thing, so either keep consistent, or rename that to commonArgs() or whatever, so they're consistent)

:)

src/main/kotlin/io/bazel/kotlin/builder/utils/ArgMap.kt Outdated Show resolved Hide resolved
@cgruber
Copy link
Collaborator

cgruber commented Mar 2, 2020

LGTM

@restingbull restingbull merged commit 226b56a into bazelbuild:master Mar 2, 2020
@restingbull restingbull deleted the optional-outputs branch March 2, 2020 20:47
cromwellian pushed a commit to cromwellian/rules_kotlin that referenced this pull request Mar 7, 2020
* Convert all Dep dependencies to be addressable by label at test time.

* Add files explicitly to runfiles from the data attribute. This is more explicit and resilient that using collect_default.

* New output scheme for the JvmCompilationTask, if an output isn't requested do not create it.

* Break apart compileAll to allow stage independent compilations.
Make jdeps optional, as it is not currently consumed so we shouldn't do the work.
Update test framework to handle the output scheme.
If kotlin srcs are absent, skip kotlin compilation.


* Introduce CompilationArgs to improve compilation argument readability

Issue: bazelbuild/issues/232
cgruber added a commit to cgruber/rules_kotlin that referenced this pull request Apr 14, 2020
* upstream/master:
  Fix non-reproducible archives (bazelbuild#304)
  Adds a kt_plugin rule (bazelbuild#308)
  Ensure that KotlionBuilder workers use a clean directory for each compilation. (bazelbuild#298)
  Apply autoformatting to all files. (bazelbuild#302)
  Optional outputs (bazelbuild#291)
  Change plugins to use depsets, as opposed to lists. (bazelbuild#292)
  Add Corbin to the codeowners. (bazelbuild#293)
  Update Protobuf to 3.11.3 (bazelbuild#286)
  Remove tree artifacts (bazelbuild#287)
  Cleanup src tree (bazelbuild#288)
  Update README.md (bazelbuild#285)
  Filter non-kotlin code out of generated sources (bazelbuild#263)
  Update readme so the dev instructions highlight using a local clone (bazelbuild#283)
  Remove third_party checked in jars, and properly pull maven dependencies. (bazelbuild#279)
  Only propagate srcjar if it isn't the default empty jar added in ae70089 to fix bazelbuild/intellij#1616 (bazelbuild#276)
  Allow resources to be in a kotlin directory (bazelbuild#268)
jongerrish added a commit to jongerrish/rules_kotlin that referenced this pull request Apr 16, 2020
* Convert all Dep dependencies to be addressable by label at test time.

* Add files explicitly to runfiles from the data attribute. This is more explicit and resilient that using collect_default.

* New output scheme for the JvmCompilationTask, if an output isn't requested do not create it.

* Break apart compileAll to allow stage independent compilations.
Make jdeps optional, as it is not currently consumed so we shouldn't do the work.
Update test framework to handle the output scheme.
If kotlin srcs are absent, skip kotlin compilation.


* Introduce CompilationArgs to improve compilation argument readability

Issue: bazelbuild/issues/232
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: jvm P1 type: cleanup Refactorings, idiomatic transforms, tech debt payoff type: performance Performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants