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

Groovy Extension Module Descriptor Transformer #81

Merged
merged 6 commits into from Aug 18, 2014

Conversation

Projects
None yet
2 participants
@mhurne
Contributor

mhurne commented Aug 8, 2014

Currently, the ServiceFileTransformer is applied to Groovy extension module descriptor files (META-INF/services/org.codehaus.groovy.runtime.ExtensionModule). Unfortunately, the format of these files does not lend itself to the strategy used by the ServiceFileTransformer, which is to concatenate the contents of all found descriptor files into a single descriptor file. Since extension module descriptor files are properties files, I considered making use of PropertiesFileTransformer, but currently it isn't flexible enough for this usage. Perhaps if PropertiesFileTransformer supported specifying mergeStrategy and mergeSeparator on a per-property basis rather than only a per-path basis, it could be used to merge extension module descriptors. However, I think a new Transformer specifically for this purpose makes more sense, as it is likely a common need (or will be in the future).

@mhurne

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 5, 2014

Owner

Hm interesting....I'm not a opposed to a merge strategy type thing. I wonder if ServiceFIleTransformer should just know about these instances though otherwise we may run into issues with ordering of the transformers (currently the first transformer that can apply to a file is used).

Owner

johnrengelman commented Aug 5, 2014

Hm interesting....I'm not a opposed to a merge strategy type thing. I wonder if ServiceFIleTransformer should just know about these instances though otherwise we may run into issues with ordering of the transformers (currently the first transformer that can apply to a file is used).

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 5, 2014

Contributor

I wondered about that. Do you think it'd be better to handle this in ServiceFileTransformer directly, or should ServiceFileTransformer ignore these files and let a new GroovyExtensionModuleDescriptorTransformer handle them?

Contributor

mhurne commented Aug 5, 2014

I wondered about that. Do you think it'd be better to handle this in ServiceFileTransformer directly, or should ServiceFileTransformer ignore these files and let a new GroovyExtensionModuleDescriptorTransformer handle them?

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 5, 2014

Contributor

By the way, there may be other files in META-INF/services that need special handling. A more general strategy for having ServiceFileTransformer leave certain service files alone for handling by other Transformers may be in order. For example, META-INF/services/org/apache/camel/component.properties.

Contributor

mhurne commented Aug 5, 2014

By the way, there may be other files in META-INF/services that need special handling. A more general strategy for having ServiceFileTransformer leave certain service files alone for handling by other Transformers may be in order. For example, META-INF/services/org/apache/camel/component.properties.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 5, 2014

Owner

Let's do something like this.

  1. Add a list of ignored patterns to ServiceFileTransformer and default it to the paths for the Groovy Extension.
  2. Add methods to ServiceFileTransformer to exclude certain patterns
  3. Add a new GroovyExtensionModuleTransformer
Owner

johnrengelman commented Aug 5, 2014

Let's do something like this.

  1. Add a list of ignored patterns to ServiceFileTransformer and default it to the paths for the Groovy Extension.
  2. Add methods to ServiceFileTransformer to exclude certain patterns
  3. Add a new GroovyExtensionModuleTransformer
@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 5, 2014

Owner

Potentially we could even just use a PatternSet in ServiceFileTransformer, then you'd get the full include/exclude capability.

Owner

johnrengelman commented Aug 5, 2014

Potentially we could even just use a PatternSet in ServiceFileTransformer, then you'd get the full include/exclude capability.

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 5, 2014

Contributor

Ok, I'll head down that path and we'll see how it turns out!

Contributor

mhurne commented Aug 5, 2014

Ok, I'll head down that path and we'll see how it turns out!

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 5, 2014

Owner

Sounds good!

Owner

johnrengelman commented Aug 5, 2014

Sounds good!

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 8, 2014

Contributor

All set for review! The Transformer interface was modified such that canTransformResource now takes a FileTreeElement rather than a String. ServiceFileTransformer now implements PatternFilterable, which allows specification of include and exclude patterns to control which service files it operates on. By default, it includes META-INF/services/** and excludes META-INF/services/org.codehaus.groovy.runtime.ExtensionModule. A new GroovyExtensionModuleTransformer was implemented.

Contributor

mhurne commented Aug 8, 2014

All set for review! The Transformer interface was modified such that canTransformResource now takes a FileTreeElement rather than a String. ServiceFileTransformer now implements PatternFilterable, which allows specification of include and exclude patterns to control which service files it operates on. By default, it includes META-INF/services/** and excludes META-INF/services/org.codehaus.groovy.runtime.ExtensionModule. A new GroovyExtensionModuleTransformer was implemented.

transform(GroovyExtensionModuleTransformer)
}
/**

This comment has been minimized.

@johnrengelman

johnrengelman Aug 8, 2014

Owner

Add these methods to the ShadowSpec interface.

@johnrengelman

johnrengelman Aug 8, 2014

Owner

Add these methods to the ShadowSpec interface.

This comment has been minimized.

@mhurne

mhurne Aug 8, 2014

Contributor

Ahh, missed that. Done!

@mhurne

mhurne Aug 8, 2014

Contributor

Ahh, missed that. Done!

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 15, 2014

Contributor

Good morning! Are there additional improvements I can make to these changes?

Contributor

mhurne commented Aug 15, 2014

Good morning! Are there additional improvements I can make to these changes?

johnrengelman added a commit that referenced this pull request Aug 18, 2014

Merge pull request #81 from mhurne/feature/groovy-extension-module-de…
…scriptor-transformer

Groovy Extension Module Descriptor Transformer

@johnrengelman johnrengelman merged commit 7009761 into johnrengelman:master Aug 18, 2014

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 18, 2014

Contributor

Thanks, John!

Contributor

mhurne commented Aug 18, 2014

Thanks, John!

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 18, 2014

Owner

Thank you. I have a few more things I want to look at before cutting a release.

Owner

johnrengelman commented Aug 18, 2014

Thank you. I have a few more things I want to look at before cutting a release.

@mhurne

This comment has been minimized.

Show comment
Hide comment
@mhurne

mhurne Aug 18, 2014

Contributor

Sounds good. I'm watching the package on Bintray, so I'll just wait for the notification.

Contributor

mhurne commented Aug 18, 2014

Sounds good. I'm watching the package on Bintray, so I'll just wait for the notification.

@johnrengelman johnrengelman added this to the 1.1.0 milestone Aug 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment