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

[MNG-8395] Add a <Source> element in the model. #1936

Merged
merged 3 commits into from
Jan 26, 2025

Conversation

desruisseaux
Copy link
Contributor

@desruisseaux desruisseaux commented Nov 25, 2024

JIRA issue: MNG-8395

Add a <Source> element in the model. Properties are:

  • directory (inherited from FileSet)
  • includes (inherited from PatternSet)
  • excludes (inherited from PatternSet)
  • scope
  • lang
  • module
  • targetVersion
  • targetPath (taken from <Resource>)
  • filtering (taken from <Resource>)
  • enabled

This commit also renames source parameter value in reader-stax.vm for avoiding name collision with the new Source model element.

@desruisseaux desruisseaux marked this pull request as draft November 25, 2024 13:20
@gnodet
Copy link
Contributor

gnodet commented Nov 25, 2024

Error in javadoc:
https://github.com/apache/maven/actions/runs/12010924190/job/33478988107#step:11:4026

- directory (inherited from FileSet)
- includes  (inherited from PatternSet)
- excludes  (inherited from PatternSet)
- scope
- lang
- module
- targetVersion
- targetPath (taken from <Resource>)
- filtering (taken from <Resource>)
- enabled

This commit also renames `source` parameter value in `reader-stax.vm`
for avoiding name collision with the new `Source` model element.

apache#1936
<p>If a module name is specified for resources or script files,
then this value modifies the directory where the files will be copied.
For example, if a Java module name is "foo.biz", then the {@code foo/bar.properties}
resource file will be copied as {@code foo.biz/foo/bar.properties}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a Maven convention, a Java convention, or something imposed by the JVM ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a Java compiler convention. In a multi-modules project, javac automatically inserts module names at the root of class directories. For example, if a module named my.foo.module contains a foo.bar.biz.Bla class, then in single-module project (as today), javac generates the following file in the Maven target/classes/ directory:

  • foo/bar/biz/Bla.class

But in a multi-modules project (would be optional and not the default), javac generates the same file at the following location instead:

  • my.foo.module/foo/bar/biz/Bla.class

Other Java tools such as java understand this directory hierarchy. I.e. the --module-path can be a directory of modules, with no need to enumerate on the path each module found in the directory. I'm not sure if it is a requirement that the directory names match module names, but this is what javac does at least by default.

Above documentation of module element tries to match what javac does. It could be rephrased as "During Java compilation, we let javac do whatever it wants. For resources, we try to copy them in the same directory as javac". The current wording tries to be more specific about what happens, but the goal is to match javac and the documentation would be updated if we find mismatches.

<version>4.1.0+</version>
<description>
<![CDATA[
Whether the directory described by this source element should be included in the build.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so. I wasn't aware of that. How should we express that?

@gnodet
Copy link
Contributor

gnodet commented Nov 25, 2024

I like it a lot, this would solve a number of problems.
I think we'd need to deprecate the previous resources, sourceDirectory, scriptSourceDirectory and testSourceDirectory in the 4.x model.

@gnodet gnodet added this to the 4.1.0 milestone Nov 25, 2024
Rename `filtering` as `stringFiltering` for avoiding confusion with file filtering.
@gnodet gnodet changed the title Add a <Source> element in the model. [MNG-8395] Add a <Source> element in the model. Nov 29, 2024
@gnodet
Copy link
Contributor

gnodet commented Jan 23, 2025

I wonder if we should include in next RC. @cstamas ?
Though it would require a bit more work to fix the deprecation and migration stuff. We've done a similar thing for module -> subproject.

@desruisseaux
Copy link
Contributor Author

Another work needed is to ensure that the new <source> tag is propagated by classes such as DefaultModelPathTranslator.java. I started that work in Geomatys@5c625d3 but didn't tested it yet (I have been delayed by other work lately, but hope to be more active in March).

@cstamas
Copy link
Member

cstamas commented Jan 23, 2025

I wonder if we should include in next RC. @cstamas ? Though it would require a bit more work to fix the deprecation and migration stuff. We've done a similar thing for module -> subproject.

I'd say go for it. We have many other things to sort out anyway. @gnodet @desruisseaux

@desruisseaux desruisseaux marked this pull request as ready for review January 26, 2025 13:52
@gnodet gnodet modified the milestones: 4.1.0, 4.0.0-rc-3 Jan 26, 2025
@gnodet gnodet merged commit 023b014 into apache:master Jan 26, 2025
13 checks passed
@desruisseaux desruisseaux deleted the feat/source branch January 26, 2025 14:39
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

Successfully merging this pull request may close these issues.

3 participants