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

Bring CompilerBuildContext to this library as part of the default build context #13

Open
virtuald opened this issue Nov 12, 2015 · 8 comments

Comments

@virtuald
Copy link
Contributor

The use case I'm thinking of is for generating java sources from the AVRO IDL (and I'm sure other IDL source generators have this same issue). The IDL specifies an 'import' statement that allows building a set of files using multiple inputs -- so it would be good to have a built-in way of saying:

  • A single input generates zero or more outputs (this is already there)
  • An input has zero or more dependent inputs that affect its output

It seems like you already have to do this sort of thing for a compiler, so extracting the CompilerBuildContext from the takari-lifecycle project and making it more generic and bringing it into the default build context would be useful.

@ifedorenko
Copy link
Contributor

Java does not have notion of source 'include'. There are type imports, but they are quite different from source includes.

We tried to model source imports as kind of input-to-input association when we looked at antlr support in the past, but API was clumsy and we didn't really need anltr support, so we didn't implement source includes.

I suggest you introduce new AbstractBuildContext subclass with the behaviour that you need, then we can see if we can make a general API in incrementalbuild library.

@virtuald
Copy link
Contributor Author

Ok, I'll give it a shot then.

I'm not super familiar with how maven magically creates the BuildContext objects, looks like.. just need a MojoExecutionScoped annotation? Though, it looks like maybe I can define a custom one if I can change the provider somehow... probably another magic annotation?

@ifedorenko
Copy link
Contributor

You need @MojoExecutionScoped annotation and pass injected BuildContextEnvironment instance to AbstractBuildContext constructor. See CompilerBuildContext for an example. This should be enough to have context instances created right before mojo execution starts and properly closed after mojo execution is done.

@virtuald
Copy link
Contributor Author

Ok, so I'm at a point where I know what information I need to store so I can do this, but I'll have to get to it tomorrow. I think the right API for what I'm looking for is something like:

 ResourceMetadata r = registerInput(f1)
 ...
 r.associateIndirectInput(f2)

And the idea would be that internally, there's yet another list of inputs. So if a change happens to f2, then it would cause f1 to be returned from registerInput() during an incremental build. The key thing is that it would not cause f2 to be returned from registerInput(), unless registerInput was also called with f2 at some point.

This seems pretty intuitive to me from an API perspective, is there something I missed that you guys ran into when you were playing with ANTLR?

It seems like it would be easier to just change this library directly, and push my fork to you. This would require adding methods to the interface for ResourceMetadata, I'm not sure if that's a change you're willing to make.

@ifedorenko
Copy link
Contributor

We used to have Input#associateIncludedInput(File). We don't have separate Input interface any more, but otherwise I believe the idea was pretty close to what you suggest.

There were few problems with that. First, #getStatus() behaviour was confusing because in some cases we needed "shallow" status of the input itself and "deep" status that covered included inputs in other cases. It was also not clear how #associateIncludedInput should behave in various input-to-output association scenarios (e.g., "multiple inputs are aggregated into single output", "input is used to generate output, which in turn is used to generate another output"). I think we simply have not looked at enough usecases to decide how to approach this yet.

So I still think you should start with a custom build context implementation. Introduce #associateIncludedInput(Resource<File> main, File included) method on the context and see if you can make it work consistently for your usecase. You most likely will need to make changes to DefaultBuildContextState, but I don't know what kind of change will work well for you.

@virtuald
Copy link
Contributor Author

To resolve the problem of getStatus, it seems like semantically instead of MODIFIED it should be NEEDSPROCESSING or DIRTY (or a better name).

For "input is used to generate another output which is used as an input", well, you already have a check in there to make sure that isn't allowed. ;)

For your multiple inputs aggregated into single output -- isn't there a separate context that takes care of that sort of thing? I haven't looked too carefully at it yet, though it's strange that it's not part of the default build context.

I'll play with it some more today and see if I can come up with something that doesn't involve a lot of copy/paste, but I don't think that will be easy to do and a fork will be easier. But we will see.

@ifedorenko
Copy link
Contributor

Maybe we should reconsider use of the same Resource interface by all context flavours, although I don't know how to do this without making things very confusing.

ResourceStatus is the status of the resource compared to the previous build. On the other hand, whether resources should be processed or not is not a resource property, but rather how the resource is used by a processor. For example, the same resource can be used to generate two outputs by the same processor, an aggregate sources zip and per-input html doc file. I strongly very believe resources status should be the same in both cases even if the html doc file must be regenerated (say because of dependencies change) and sources zip file should remain the same (none of the sources changed).

@virtuald
Copy link
Contributor Author

Alright, you were right -- that was pretty easy once I found everything I needed, and it seems to work in the limited testing I've done. Here's a gist of my object -- pretty short:

https://gist.github.com/virtuald/77d14ad1f01ff61cd20e

This is a really useful library, thanks so much for the work! I'll be exploring the other lifecycle pieces too, but it seems pretty solid so far.

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

No branches or pull requests

2 participants