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

[rsc-compile] define workflow in context instead of on fly; use workf… #7324

Merged
merged 5 commits into from Mar 15, 2019

Conversation

Projects
None yet
3 participants
@baroquebobcat
Copy link
Contributor

commented Mar 7, 2019

…low in traversal

This pulls the workflow up into the context objects and draws them through to the places we classify targets. This way we're centralizing how we do classification and only doing it once per target.

It also fixes an issue with how we were handling the invalid dep traversal where we were depending on the sources of targets in the predicate, instead of the workflow

baroquebobcat added some commits Mar 7, 2019

[rsc-compile] define workflow in context instead of on fly; use workf…
…low in traversal

This pulls the workflow up into the context objects and draws them through to the places we classify targets. This way we're centralizing how we do classification and only doing it once per target.

It also fixes an issue with how we were handling the invalid dep traversal where we were depending on the sources of targets in the predicate, instead of the workflow
@cosmicexplorer
Copy link
Contributor

left a comment

Just a note that the reason I wanted to use an @memoized_method for _classify_compile_target() was so we could avoid doing the classification more than once per target, but I think this formulation is definitely better and easier to extend in a v2 context.

It also fixes an issue with how we were handling the invalid dep traversal where we were depending on the sources of targets in the predicate, instead of the workflow

Could you please expand on what this is referring to? Is this coming from the perception that _classify_compile_target() wasn't memoized?

@@ -125,6 +125,9 @@ def classify_target(cls, target):
target_type = None
return target_type

_JvmCompileWorkflowType.rsc_then_zinc = _JvmCompileWorkflowType('rsc-then-zinc')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

This should be unnecessary post #7269!

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Oh, so if I changed the type names to underscore separated, then those class attrs would just work?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

So, that should be true, but also, I got the PR number wrong -- as of #7304, you should be able to keep them separated by hyphens (the recommended convention), and have the attributes generated with underscores!

if self._classify_compile_target(target) is not None:
return True
return False
return target.has_sources('.java') or target.has_sources('.scala')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

I would still like it if we were to use self._classify_compile_target() here instead of having more one-off conditionals, it means there's one place to check if errors occur.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Hm. That's a fair point.

If a necessary dep is a Scala dep and the root is Java, continue to recurse because
otherwise we'll drop the path between Zinc compile of the Java target and a Zinc
compile of a transitive Scala dependency.
If a necessary dep is a Rsc compiled dep and the root is a Zinc one, continue to recurse because

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

The terms "rsc-compatible" and "zinc-only" seem useful here to distinguish between compiler invocations (involving both rsc and zinc) and target types (strictly one or the other).

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

oh, as in, use the workflow names instead of plain language? I think that's reasonable.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

Yes! But hopefully these distinctions can go away soon enough with further work on rsc compatibility / automatic rewriting.

'zinc-only': False
}),
'rsc-then-zinc': lambda : False
})()

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

After #7269, this could be:

    return contexts[compile_target][0].workflow.resolve_for_enum_variant({
      'zinc-only': lambda : contexts[dep][0].workflow == _JvmCompileWorkflow.rsc_then_zinc,
      'rsc-then-zinc': lambda: False,
    })()

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

I'll convert compile_contexts into a datatype in followup PR.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Cool.

This is the bit that I was referring to as a bug fix.

Could you please expand on what this is referring to?
It refers to the fact that this conditional was trying to model different workflows, but when I adjusted how workflows were chosen, I hadn't updated this.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

Oh oops, thank you for elaborating!

@stuhood stuhood added this to the 1.15.x milestone Mar 13, 2019

@cosmicexplorer
Copy link
Contributor

left a comment

Forgot to give the right PR number -- as of #7304 we should be able to use zinc-only and rsc-then-zinc (with hyphens) and have the underscored class attributes generated for us. To reduce diff churn (and because hyphens are the preferred style for enum string values), I would recommend reverting to using hyphens (sorry!), but this is great, thanks for being open to the feedback!

@baroquebobcat

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Alright. I'll do one more pass and use -s again.

@baroquebobcat baroquebobcat merged commit 4b3b7b7 into pantsbuild:master Mar 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:nhoward/rsc_change_workflow branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.