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

[rsc-compile] further fixes #6745

Merged
merged 6 commits into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@baroquebobcat
Contributor

baroquebobcat commented Nov 8, 2018

  • clean up variable names and ordering
  • ensure that only java targets are passed to the rsc metacp stage
  • remove the workaround tool classpath, and use the stub flag
  • pull metacp inputs from compile_classpath instead of rsc_classpath in some cases
@stuhood

stuhood approved these changes Nov 9, 2018

# providing metacp'd
# - jar deps
# - non-java deps
# - metacp'd java deps

This comment has been minimized.

@xeno-by

xeno-by Nov 9, 2018

Contributor

Can we have tickets open for all these things? Both JIRA and GitHub would work.

This comment has been minimized.

@baroquebobcat

baroquebobcat Nov 9, 2018

Contributor

I'm realising this comment isn't clear as to purpose. This describes what's going on below it.

strict_dependencies_for_target = list(DependencyContext.global_instance().dependencies_respecting_strict_deps(target))

This comment has been minimized.

@xeno-by

xeno-by Nov 9, 2018

Contributor

From what I can see, strict_dependencies_for_target don't have to be strict dependencies, do they? If my understanding is correct, perhaps this variable could be renamed?

This comment has been minimized.

@baroquebobcat

baroquebobcat Nov 9, 2018

Contributor

They are not necessarily strict_deps. So I could use a better name here. I'll update it.

# Step 1: Convert classpath to SemanticDB
# ---------------------------------------
# If there are any un-metacp'd dependencies, metacp them so their indices can be passed to
# Rsc.

This comment has been minimized.

@xeno-by

xeno-by Nov 9, 2018

Contributor

What does un-metacp'd mean?

This comment has been minimized.

@baroquebobcat

baroquebobcat Nov 9, 2018

Contributor

When we are in this context, Java dependencies may not have been run through metacp yet. This section here runs those through metacp. #6754

# TODO use compile_classpath
classpath_abs = [
path for (conf, path) in
self.context.products.get_data('rsc_classpath').get_for_target(ctx.target)
#self.context.products.get_data('rsc_classpath').get_for_target(ctx.target)

This comment has been minimized.

@xeno-by

xeno-by Nov 9, 2018

Contributor

Can this comment be removed?

This comment has been minimized.

@baroquebobcat
output_dir=rsc_index_dir)
metacp_stdout = stdout_contents(metacp_wu)
metacp_result = json.loads(metacp_stdout)
metacp_result = json.loads(stdout_contents(metacp_wu))

This comment has been minimized.

@xeno-by

xeno-by Nov 9, 2018

Contributor

Looks like rsc_compile.py is gradually turning into a wall of text (it's close to 1kloc right now). I don't know about coding conventions in this project, so feel free to ignore this request, but perhaps it would be beneficial to split this logic into multiple modules (e.g. by splitting the rsc_compile task into multiple tasks such as metacp, rsc and scalac)?

This comment has been minimized.

@baroquebobcat

baroquebobcat Nov 9, 2018

Contributor

Breaking this up is on my todo list. One of the issues is that I don't want to break it up into separate v1 style tasks because I don't think that buys much.

I'm planning on doing a refactoring pass on it once I have a good feel for the bones of the abstractions that are needed, but I want to leave it a bit too verbose at the moment.

@baroquebobcat baroquebobcat changed the title from WIP TODO Tests [rsc-compile] further fixes to [rsc-compile] further fixes Nov 15, 2018

@baroquebobcat baroquebobcat merged commit 49cc12e into pantsbuild:master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment