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

Map "build" phase to asciidoctor-maven-plugin #27

Closed
abelsromero opened this issue Aug 18, 2019 · 20 comments
Closed

Map "build" phase to asciidoctor-maven-plugin #27

abelsromero opened this issue Aug 18, 2019 · 20 comments
Assignees

Comments

@abelsromero
Copy link

Regardless of how we/if we end up merging the lifecycle with the asciidoctor-maven-plugin, I am wondering if there's any reason not to have it mapped by default.
I think that it would be nice if just by adding the lifecycle one could already run mvn build and be able to convert some docs provided sources match with the plugin's defaults.

@rrialq
Copy link
Member

rrialq commented Aug 18, 2019

The main reason is that if you define a shared configuration and several executions, an additional document corresponding to the default backend (dockbook) will be generated.
So till more investigation about that, I decided not mapping it.
I think that documenting the way of sharing configuration between executions and using the main configuration to one of the backends solve this undesired conversion, and the process-asciidoc goal could be mapped in the build phase.

@abelsromero
Copy link
Author

I just tested it and that's true! We could comment that with the maven guys at https://issues.apache.org/jira/. This behavior does not make sense to me and does not match the behavior when configuring a normal plugin not mapped to lifecycle. But I see it also happens for default plugins, so I don't have much hope here, for example:

<plugin>
	<artifactId>maven-compiler-plugin</artifactId>
	<version>3.8.0</version>
	<executions>
		<execution>
			<id>custom</id>
			<goals>
				<goal>compile</goal>
			</goals>
			<phase>compile</phase>
		</execution>
	</executions>
</plugin>

will show this in the console

[INFO] --- maven-compiler-plugin:3.8.0:compile (default-compile) @ my-app ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 1 source file to /home/asalgadr/temp/my-app/target/classes
[INFO] 
[INFO] --- maven-compiler-plugin:3.8.0:compile (custom) @ my-app ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 1 source file to /home/asalgadr/temp/my-app/target/classes

Btw, Docbook won't be default backend in the next 2.0.0 release (asciidoctor/asciidoctor-maven-plugin#188).

@abelsromero
Copy link
Author

Note, same behavior for clean plugin :/

@rrialq
Copy link
Member

rrialq commented Aug 19, 2019

Yes, I know of that behaviour of the default plugins,but I was worried about the time needed for Asciidoc for doing a conversion not specified by the user in the configuration.
In my opinion, if the user doesn't configure a backend explicitly, the asciidoctor-maven-plugin should skip the conversion.
But as this goes out of the asciidoctor-lifecycle-maven-plugin I will map the process-asciidoc to the build phase by default.

@abelsromero
Copy link
Author

doing a conversion not specified by the user in the configuration.

This is what concerns me, more than time. If executions are added to the default one instead of replacing it, output will not be the expected. So we should probably stick to your original idea of NOT mapping it.
It seems weird to me, but I don't think running asciidoctor-maven-plugin with only defaults is normal. So it's fine as it is right now...I guess 🤷‍♂️

@rrialq
Copy link
Member

rrialq commented Aug 19, 2019

This is what concerns me, more than time. If executions are added to the default one instead of replacing it, output will not be the expected.

  • When you map a goal to a phase, it will be executed with the plugin configuration, regardless of the executions.
  • If the goal is not mapped to a phase, only the defined configurations setted in executions tag will be executed.

I think this can be managed in many ways, depending of mapping or not the build goal to the build phase.

  • Moving the configuration of one of the executions to the plugin configuration. But I don't know what should be the collateral effects (a parameter not needed by one of the executions, for example).
  • Or with the help of the skip parameter of asciidoctor-maven-plugin.

In my opinionit would be more appropiate mapping the build goal to the build phase, to avoid setting the phase in the executions, because it has no sense that you define a lifecycle where the main goal needs to be mapped explicitily.

@abelsromero
Copy link
Author

Already tried with skip, and did not help. It is required to set it to true in and then false in each execution, or else they inherit the value.

In my opinionit would be more appropiate mapping the build goal to the build phase, to avoid setting the phase in the executions, because it has no sense that you define a lifecycle where the main goal needs to be mapped explicitily.

Not sure I follow. Everything I see seems to indicate that mapping it causes more trouble than helps, but you are implying the opposite?

@rrialq
Copy link
Member

rrialq commented Aug 19, 2019

No, I am saying that the mapping should not try to fix that problem, because is not a problem of asciidoctor-lifecycle-maven-plugin, so mapping should be done.
So the fix should be one of:

  • Modify asciidoctor-maven-plugin to avoid generation if no explicit backend is configured.
  • Or setting the plugin configuration for one format, and one execution for each other formats desired.

None of the fixes goes into asciidoctor-lifecycle-maven-plugin, so I am considering proposing to guys of asciidoctor-maven-plugin that when no backend specified, no generation is done.

@abelsromero
Copy link
Author

None of the fixes goes into asciidoctor-lifecycle-maven-plugin, so I am considering proposing to guys of asciidoctor-maven-plugin that when no backend specified, no generation is done.

The problem is that no only this breaks backward compatibility (but this is a minor issue really), but it goes against the "convention over configuration" principle of Maven (main point for me here).
Don't mean to be rude, but I am gonna be pretty intransigent here 😈 a maven plugin should avoid at all cost having required parameters.

@rrialq
Copy link
Member

rrialq commented Aug 20, 2019

The problem is that no only this breaks backward compatibility (but this is a minor issue really)...

You must understand that when I told you about a proposal, I was still thinking about the problem, so I told you only the main idea, and not the full proposal.

In this case it is easy to keep backward compatibility, with a few code lines. I haven't taken a look at the asciidoctor-maven-plugin code, but I am sure it should be easy to add the new feature keeping backward compatibility.

..., but it goes against the "convention over configuration" principle of Maven (main point for me here).

Ok, I think the same as you, but with some slight discrepancies.

  • The maven-site-plugin forces you to write a site.xml file, although in many cases a default content could be assumed for generating basic site. Why the plugin doesn't supply a default site.xml?
  • The maven-surefire-plugin adds in the 2.4 version the parameter failIfNoTests, that help to modify the functionality without breaking backward compatibility.

Convention over configuration, but flexibility to change functionality above all.

... Don't mean to be rude, but I am gonna be pretty intransigent here 😈 a maven plugin should avoid at all cost having required parameters.

I respect your opinion, of course, although I disagree on this: I follow this little known saying:

Never let your sense of morals prevent you from doing what is right (Asimov).

Please accept my apologies. It is possible that I had expressed myself wrong. I didn't mean that the plugin should be changed for requiring parameters that currently are optional. When I said :

Modify asciidoctor-maven-plugin to avoid generation if no explicit backend is configured.

I wanted to say that it should be studied, and after thinking a bit about it, in my opinion:

If the modification is designed to keep it, for example, with a new boolean parameter named skipProcessIfNoBackend, with default value false., so:

  • In case the value of this parameter is true and no value for backend was configurated, then asciidoctor should not process files.
  • In case the value of this parameter is false or this parameter is not present in plugin (or execution) configuration (projects that already are using the asciidoctor-maven-plugin) asciidoctor should process files.
  • The projects that already are using the asciidoctor-maven-plugin would not have to change the settings, because the default vlue of skipProcessIfNoBackend will mean the same as the current version.

So the plugin asciidoctor-maven-plugin would work as nowdays without the skipProcessIfNoBackend parameter in their configurations.

Now I think I would be wrong to propose this change because of the opposition seen in this informal conversation about it, even before a serious study and a full-fledged proposal.
It is not my style, I do not make proposals lightly, much less break the compatibility (unless there is no alternative).

@abelsromero
Copy link
Author

abelsromero commented Aug 20, 2019

DISCLAIMER: when long posts appear it seems one is getting angry or something like that :P don't worry, not the case. Just trying to explain points in detail, and keep in mind that written comunication is more complicated that oral.

I feel the conversation shifted.
The thing for me was whether the maven-plugin should be mapped by default or not. I though it was a good idea so the lifecycle allowed generated some output with no extra configuration other than the lifecycle.

PROS

  • Converts documents by default according to current maven-plugin defaults. No need to add the maven-plugin to the pom unless custom configuration wants to be applied (even though, adding config is probably most of cases).

CONS

  • "In my opinion", side effects when using executions. That is because the default execution is added to the current ones. That means that if someone sets 2 executions, there will be 3 (always N+1 executions).
    Note 1: this does not happen when using the maven-plugin outside of a custom lifecycle.
    Note 2: when not using executions, the configuration is applied to the default one. Another minor PRO here, we simplify configuration because there's no need to map the plugin manual to any goal, this aligns with the overall idea of providing meaningful defaults.

From that point, we can either decide a) not to map it or look for a b) way to disable the default execution.

a) I was finally convinced not to do so, but I would really like to offer something more than an empty lifecycle.
b) Here is were we don't agree, and that fine 😄 let's work on it.
I don't like adding new parameters like the ones proposed, it is hacky and I don't think the arguments apply here (site.xml is optional in fact, and failIfNoTests makes sense since surefire has a clear simple goal and it's failing, not skiping). Making the build skip based on a single configuration that is optional in all other asciidoctor modules (ruby gem, asciidoctorj, etc.) would be, in my opinion, counter intuitive for users.
Just in case we explore the option of applying different configurations for the plugin inside the lifecycle. I don't know if it's possible, but even if we can modify some settings from the lifecycle to change the defaults, it means digressing from standard Maven components (see clean & compile examples before). I really don't like it, but the fact that executions are added to default executions is a default Maven pattern. To be pedantically clear, I don't like it, I think it should not behave like that, and I am considering asking in maven forums to understand reasons why...but still, I think we should follow the conventions.
On the other hand, and as you mention in the first post maybe we can fix this with documentation, just tested using <skip> like show below, and worked like a charm. Maybe that could be an option?

 <configuration>
    <skip>true</skip>
    <sourceDirectory>src/docs/asciidoc</sourceDirectory>
    <attributes>
        <endpoint-url>http://example.org</endpoint-url>
        <sourcedir>${project.build.sourceDirectory}</sourcedir>
        <project-version>${project.version}</project-version>
    </attributes>
</configuration>
<executions>
    <execution>
        <id>asciidoc-to-html</id>
        <configuration>
            <skip>false</skip>
            <backend>html5</backend

Worst case scenario, someone won't apply to the default configuration and will eventually find the error, then come to ask at the repo or check the docs to find the answer. The maven-plugin generates a line for each doc converted, so it's not hard to spot if an extra execution is being done.

@rrialq
Copy link
Member

rrialq commented Aug 21, 2019

DISCLAIMER: when long posts appear it seems one is getting angry or something like that :P don't worry.

Now that you have put on the table the matter about the tone of the conversation, I consider it necessary to clarify a few questions in this regard.

I am almost fifty years old.
I am too old to follow the pseudo conventions of young people about Internet conversations.
I don't have Twitter account, I don't have Facebook account or Instagram, or Skype, I don't need them.

  • I believe in freedom of expression with respect. I do not believe in attributing it to other third intentions based on interpretations of phrases.
  • I express myself directly, I do not go around or subterfuge, if I have to express my anger I will say "this expression has angered me".
  • I beg you not to confuse my exercise of direct free expression with any kind of attitude, beyond the conversation itself.
  • If my posts are long it is because I consider a long explanation necessary.
  • If my posts are short it is because I think it is not necessary to write more.
  • I will respect that you do not agree with my way of being, in the same way that I respect your way of facing the conversation without trying to change it or impose conditions, much less make interpretations, but I will not respect that you try to impose rules on how to express myself.

We can both agree to repect the other's way of being and continue the collaboration or agree to settle it at this time.

I beg your pardon if this speech seems aggressive to you, but I try to prevent misunderstandings in the future.

@rrialq
Copy link
Member

rrialq commented Aug 21, 2019

I don't like adding new parameters like the ones proposed, it is hacky and I don't think the arguments apply here (site.xml is optional in fact, and failIfNoTests makes sense since surefire has a clear simple goal and it's failing, not skiping)

  • site.xml: OK, you're right. It is no needed. May be this changed with 3.x released version of the plugin?
  • failIfNoTests. I put an incorrect example about what I wanted to say.

@rrialq
Copy link
Member

rrialq commented Aug 21, 2019

About how configure the plugin to avoid the default backend generation, in my opinion we can configure one of the backends in the plugin configuration, with the default shared configuration, and the other backends in executions, overwriding the plugin configuration when necessary.

With this configuration, no execution is losted (we avoid skip the main execution), and we reduce the number of executions.

@abelsromero
Copy link
Author

Can you provide an example? If I get it correctly that's how it is now.

@rrialq
Copy link
Member

rrialq commented Aug 22, 2019

The following configuration will be valid for html and pdf (I have omitted non relevant configuration).

<plugin>
    <groupId>org.asciidoctor</groupId>
    <artifactId>asciidoctor-maven-plugin</artifactId>
    <version>${asciidoctor.maven.plugin.version}</version>              
    <configuration>
        <!-- Shared configuration -->
        <sourceDirectory>src/docs/asciidoc</sourceDirectory>
        <sourceHighlighter>coderay</sourceHighlighter>
        <!-- Specificy HTML5 configuration -->
        <backend>html5</backend>
        <attributes>
            <!--  Shared attributes-->
            <sourcedir>${project.build.sourceDirectory}</sourcedir>
            <project-version>${project.version}</project-version>
            <imagesdir>./images</imagesdir>
            <icons>font</icons>

            <!-- HTML configuration -->
            <docinfo1>true</docinfo1>
            <idprefix/>
            <idseparator>-</idseparator>
            <sectanchors>true</sectanchors>
            <toc>left</toc>
        </attributes>
    </configuration>
    <executions>
        <execution>
            <id>generate-pdf-doc</id>
            <phase>build</phase>
            <goals>
                <goal>process-asciidoc</goal>
            </goals>
            <configuration>
                <backend>pdf</backend>
                <attributes>
                    <docinfo1>false</docinfo1>
                    <idprefix/>
                    <idseparator>-</idseparator>
                    <pagenums/>
                    <toc/>
                    <sectanchors>false</sectanchors>
                </attributes>
            </configuration>
        </execution>
    </executions>
</plugin>

So instead of having a shared configuration in plugin configuration and two executions (one for HTML5 and another for pdf) we have a plugin configuration that contains the shared configuration and the HTML5 configuration, and one execution with the pdf configuration.

@rrialq
Copy link
Member

rrialq commented Aug 22, 2019

That configuration is the same as I said 4 days before, in:

I think that documenting the way of sharing configuration between executions and using the main configuration to one of the backends solve this undesired conversion, and the process-asciidoc goal could be mapped in the build phase.

Originally posted by @rrialq in #27 (comment)_

@abelsromero
Copy link
Author

That configuration is the same as I said 4 days before

I am aware, just want to be 100% sure.
Apologies if it seems going in circles. Even we come back to the starting point in between we explored several options and confirmed the approach.

Summary: map it and document clearly that there will be an additional execution. Nothing else.

@rrialq rrialq self-assigned this Aug 23, 2019
rrialq added a commit that referenced this issue Aug 23, 2019
@rrialq
Copy link
Member

rrialq commented Aug 25, 2019

The process-asciidoc is mapped to the build phase.
It is documented too.

@ghost
Copy link

ghost commented Jan 6, 2020

The process-asciidoc is mapped to the build phase.
It is documented too.

yes,it helps me

<asciidoctor.maven.plugin>2.0.0-RC.1</asciidoctor.maven.plugin>
<asciidoctorj.pdf.version>1.5.0-beta.8</asciidoctorj.pdf.version>

flatten-maven-plugin and lifecycle-mapping
asciidoctor-maven-plugin using <goal>process-asciidoc</goal>

thanks again!

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