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

narSystemDirectory violates 'Convention over configuration' principle #156

Open
ponomandr opened this issue Mar 14, 2015 · 7 comments
Open
Milestone

Comments

@ponomandr
Copy link

tl;dr
Change default value of narSystemDirectory from target/nar/nar-generated to target/generated-sources/nar

Rationale
By convention Maven plugins put generated sources to target/generated-sources/<something> directory. This value is so common that many IDEs recognize it by default and attach this directory as a source dir to the project. I'm not sure about Eclipse, but Intellij and Netbeans do this. Intellij IDEA even warns you if you try to modify a file in the 'generated-sources' dir.

On the other hand, Intellij IDEA marks target directory as "excluded". So the content of the directory is completely ignored (except 'generated-sources' subdirectory). Because of that, NarSystem.java is not visible for IDE and the project cannot compile.

Moreover, it's not possible to configure narSystemDirectory to point to `target/generated-sources'

@dscho
Copy link
Member

dscho commented Mar 14, 2015

Please clarify. The statement

By convention Maven plugins put generated sources to target/generated-sources/ directory. [...] many IDEs recognize it [...] Intellij and Netbeans do this.

and the statement

Intellij IDEA marks target directory as "excluded".

seem to contradict each other.

Second, if the statement

Because of that, NarSystem.java is not visible for IDE and the project cannot compile.

is true, then the suggestion to "do it the Maven way" would introduce a serious bug instead of solving one.

Lastly, IMHO

I'm not sure about Eclipse

needs a conclusive answer before going any further with this ticket, given that Eclipse is the most popular Java IDE of them all, by far.

@ponomandr
Copy link
Author

Sorry, I didn't use Eclipse for many years and my assumptions can be wrong.
I've just downloaded Eclipse and tested the plugin. Eclipse behavior differs from Intellij IDEA.

It seems that all folders in Eclipse project can have only two states: "source folder" or not (please correct me if I'm wrong).

Intellj IDEA has more advanced project structure. It distinguishes different types of folders: "source", "resources", "test source", "test resources", "generated sources" and "excluded".

When you import/update maven project in Eclipse, it detects all sources in 'target ' regardless of relative path and adds them to "Java Build Path".

When you do the same in Intellij IDEA, it sets "target" directory type to "Excluded" and content of directory is excluded from search, refactoring and other actions.

On the other hand Intellij knows that maven plugins typically put generated source code to target/generated-sources. So it makes exception for this folder and treats its subfolders as "generated sources" automatically. It is possible to mark other directory under target as 'generated sources' but if you do 'update maven project' action this configuration will be lost.

So here is the main difference between Eclipse and Intellij:
Eclipse doesn't care about path to generated sources, but Intellij IDEA do. So target/nar/nar-generated works in Eclipse, but doesn't work in Intellij.

Although 48% of developers use Eclipse, according to the same survey 49% would prefer to use Intellij IDEA. So users of this IDE should not be ignored.

@dscho
Copy link
Member

dscho commented Mar 15, 2015

When you import/update maven project in Eclipse, it detects all sources in 'target ' regardless of relative path and adds them to "Java Build Path".

This is incorrect. The NAR plugin has to do extra work to let Eclipse know about the NarSystem class.

Although 48% of developers use Eclipse, according to the same survey 49% would prefer to use Intellij IDEA. So users of this IDE should not be ignored.

I am absolutely, utterly uninterested in a discussion which IDE is better. The point is that a fix for IntelliJ that breaks Eclipse support is not a fix. If you want to support IntelliJ better, that is a laudable goal; You just have to find a way that does not break things that are Working Right Now.

@ctrueden
Copy link
Contributor

I agree that target/generated-sources would be more consistent with standard Maven practices. And changing it would be easy; see:

$ git grep 'nar-generated'
src/main/java/com/github/maven_nar/Library.java:    @Parameter(defaultValue = "${project.build.dir}/nar/nar-generated", required = true)
src/main/java/com/github/maven_nar/Library.java:    private String narSystemDirectory = "nar-generated";
src/site/apt/configuration.apt: [narSystemDirectory] Specifies the NarSystem source directory. Defaults to target/nar/nar-generated.

I also agree with @dscho that after this change, testing should be done in Eclipse, NetBeans, IntelliJ and CLI to verify that everything works in all four.

@ponomandr I would also suggest being careful with your analysis of such issues; my understanding is that both NetBeans and IDEA lean heavily on Maven CLI tools to do the build, so things are supposed to work the same as on the CLI in those IDEs. If IDEA currently has problems, it would be good to have an MCVE illustrating otherwise, rather than jumping straight to a proposed solution.

@ctrueden
Copy link
Contributor

@ponomandr I pushed a patch that implements your suggestion—which which I agree, BTW: using target/generated-sources is much more in line with Maven convention.

Could you please test it, and see if it fixes your issue with IntelliJ IDEA?

@ponomandr
Copy link
Author

@ctrueden It looks like defaultValue attribute of @Parameter is completely ignored.

When I tested this configuration:

      <plugin>
            <groupId>com.github.maven-nar</groupId>
            <artifactId>nar-maven-plugin</artifactId>
            <version>3.2.3-SNAPSHOT</version>
            <extensions>true</extensions>
            <configuration>
                <libraries>
                    <library>
                        <narSystemPackage>com.mycompany.mypackage</narSystemPackage>
                    </library>
                </libraries>
            </configuration>
        </plugin>

I got the source generated in target/nar/nar

Then I added `narSystemDirectory' configuration:

        <plugin>
            <groupId>com.github.maven-nar</groupId>
            <artifactId>nar-maven-plugin</artifactId>
            <version>3.2.3-SNAPSHOT</version>
            <extensions>true</extensions>
            <configuration>
                <libraries>
                    <library>
                        <narSystemPackage>com.mycompany.mypackage</narSystemPackage>
                        <narSystemDirectory>xxx</narSystemDirectory>
                    </library>
                </libraries>
            </configuration>
        </plugin>

The code was generated at target/nar/xxx

To make sure I use correct version of the plugin, I removed narSystemDirectory from pom.xml and changed this piece of code:

    @Parameter(defaultValue = "${project.build.dir}/generated-sources/nar", required = true)
    private String narSystemDirectory = "zzz"; // Modified this value

And the resulting output dir became target/nar/zzz

@ctrueden
Copy link
Contributor

@ponomandr OK, so the target/nar prefix must originate somewhere else. It hopefully wouldn't be too hard to track down. I warmly invite you to file a PR if you figure it out!

@ctrueden ctrueden modified the milestone: unscheduled Aug 6, 2015
@ctrueden ctrueden removed the dormant label Aug 6, 2015
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

3 participants