You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/11/09 21:08:20 UTC
[GitHub] [maven-scm] Lonzak opened a new pull request #107: Fix for SCM-946
Lonzak opened a new pull request #107:
URL: https://github.com/apache/maven-scm/pull/107
added an @threadSafe to the mojo.
Alternatively it can also be added in the concrete classes like TagMojo:
`@Mojo( name = "tag", aggregator = true, threadSafe = true )`
Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974704356
> > Perhaps plugin-plugin could add this to the site, like is done for other [attributes]
>
> It does: https://github.com/apache/maven-plugin-tools/blob/2b4756dd67138ac85a18499fc39f31767b10fd3e/maven-plugin-tools-generators/src/main/java/org/apache/maven/tools/plugin/generator/PluginXdocGenerator.java#L316
>
> But only for the positive case it seems.
You should handle the oppositve case as well, making the "not" emphesized. Happy to require a PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o closed pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #107:
URL: https://github.com/apache/maven-scm/pull/107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970411568
Ok I changed it to false.
@rmannibucau If you mean the [MojoAnnotationVisitor ](https://maven.apache.org/components/plugin-tools-archives/plugin-tools-3.0/apidocs/org/apache/maven/tools/plugin/annotations/scanner/visitors/MojoAnnotationVisitor.html) it certainly would make sense define some reasonable default or fallback value. But isn't this another maven project? (I am not so familiar with the overall project structure...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970454163
I see. Would make sense then other projects which have the same problem would be solved, too. But I see this as a parallel improvement to this one. If a plugin sets the value (then basically overriding it) it is okay I would say...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974698770
> I think we should merge this and rename the ticket to make it explicitly clear that this plugin is _not_ threadsafe.
Goals *are* already marked with threadSafe=false, there is no third option.
It would not make it more not thread safe.
It'd be _explicitly clear_ only in the code, so interested party would need to check it in the source. I don't like it.
It would have no impact on the build itself.
Perhaps plugin-plugin could add this to the site, like is done for other
> Attributes:
>
> Requires a Maven project to be executed.
> Executes as an aggregator plugin.
This would as well save from analysis and changes to _actually_ make the goals thread safe.
Hence I'm :-1: for merging.
<sup>(But I'm not maintainer here, so only sharing my opinion.)</sup>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972638227
@pzygielo Damn it - initially I had set it to true and only on the tag mojo. (Then the warning disappeared). Afterwards I thought lets add it to the parent to set it for all mojos and since nobody knew whether they were threadsafe it was changed to false (and I didn't test again)... Sorry for the confusion - so maybe the goal should be to make it threadSafe ;-)
I'll try to investigate the issue I had with the parallel execution - must be related to something different then ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970096072
What is the different between `false` and settings? I don't see any at the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971782876
@Lonzak I'd like 1 and 2 to merge as the default when nothing is specified in our plugin tools stack, this way we are always in a safe runtime zone. The runtime warning would move from the runtime to the build time (mojo dev) since it is the only location something can be done - at runtime users will suffer from it but can't do anything without patching the plugin so warning is not really helping until the runtime execution is kept concurrent which is not safe. So I think we should lean toward:
1. threadSafe=false or unspecified = considered as false at runtime, warning at build time
2. threadSafe=true = let it be concurrent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971865405
I think @pzygielo is right, this likely needs to be applied to all mojos separately. Though, have not verified it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974704060
> Perhaps plugin-plugin could add this to the site, like is done for other [attributes]
It does: https://github.com/apache/maven-plugin-tools/blob/2b4756dd67138ac85a18499fc39f31767b10fd3e/maven-plugin-tools-generators/src/main/java/org/apache/maven/tools/plugin/generator/PluginXdocGenerator.java#L316
But only for the positive case it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974704356
> > Perhaps plugin-plugin could add this to the site, like is done for other [attributes]
>
> It does: https://github.com/apache/maven-plugin-tools/blob/2b4756dd67138ac85a18499fc39f31767b10fd3e/maven-plugin-tools-generators/src/main/java/org/apache/maven/tools/plugin/generator/PluginXdocGenerator.java#L316
>
> But only for the positive case it seems.
You should handle the oppositve case as well, making the "not" emphesized. Happy to review a PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: SCM-946 - Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724545528
No that wasn't my intention as I have written in the corresponding issue SCM-946. But I agree that it could have been misleading. I updated the description.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970459864
@Lonzak agree, just would be great to not see dozens of PR to explicitly set the attribute and let external plugins benefit from it OOTB too - it is not a trivial behavior since generally annotations defaults are kept when not using this kind of bytecode parsing. But not issue with this PR for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971622055
@Lonzak not sure I got the first part, it is always possible to say it is not thread safe when not specified. Worse case you become sequential when it would be possible to be thread safe but there it is a plugin dev fix and all other cases are okish at runtime, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on a change in pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on a change in pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#discussion_r751454937
##########
File path: maven-scm-plugin/src/main/java/org/apache/maven/scm/plugin/AbstractScmMojo.java
##########
@@ -57,6 +58,9 @@
* @author <a href="evenisse@apache.org">Emmanuel Venisse</a>
* @author Olivier Lamy
*/
+
+@Mojo( name = "ScmMojo",
+ threadSafe = false)
Review comment:
```
[INFO] --- maven-checkstyle-plugin:3.0.0:check (checkstyle-check) @ maven-scm-plugin ---
[INFO] There is 1 error reported by Checkstyle 6.18 with config/maven_checks.xml ruleset.
[ERROR] src/main/java/org/apache/maven/scm/plugin/AbstractScmMojo.java:[63,26] (whitespace) ParenPad: ')' is not preceded with whitespace.
[WARNING] checkstyle:check violations detected but failOnViolation set to false
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970141490
Shouldn't MojoAnnotationVisitor be changed to ensure there is a safe default to avoid such an issue (instead of just relying on visited value, rely on spec - reflection - + visited value as overrides for values with defaults)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970649563
I believe that this change is a noop:
```java
boolean threadSafe() default false;
```
It will be `false` by default any way....
Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970454163
I see. Would make sense then other projects which have the same problem would be solved, too. But I see this as a parallel improvement to this one. If a plugin sets the value (then basically overriding it) they could do that I would say...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970414909
@Lonzak maven-plugin-tools: https://github.com/apache/maven-plugin-tools/blob/129217b00c49aaaedbcf829e272f22cb40ef3338/maven-plugin-tools-annotations/src/main/java/org/apache/maven/tools/plugin/extractor/annotations/scanner/visitors/MojoAnnotationVisitor.java#L33 . Idea can be to fix it there then upgrade maven plugin dependencies in this project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970059393
Any updates? Can I support you on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on a change in pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on a change in pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#discussion_r752018662
##########
File path: maven-scm-plugin/src/main/java/org/apache/maven/scm/plugin/AbstractScmMojo.java
##########
@@ -31,6 +31,7 @@
import org.apache.maven.plugin.AbstractMojo;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugins.annotations.Component;
+import org.apache.maven.plugins.annotations.Mojo;
Review comment:
I smell unused import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974698770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: SCM-946 - Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724545528
No that wasn't my intention as I have written in the corresponding issue SCM-946. The `@threadSafe` could also be set to false but it should be set either way to indicate a plugins capabilities. But I agree that it could have been misleading. I updated the description.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970103048
There is a big difference: when you set it to false your plugin would not be executed in parallel and the warning would not appear. I mean in case of doubt set it to false - now the behavior is undefined and the plugin thus might even be executed in parallel which could lead to failures. So explicitly set it (even if it is false)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971349942
Added this to 1.21.1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974686904
I think we should merge this and rename the ticket to make it explicitly clear that this plugin is *not* threadsafe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974704356
> > Perhaps plugin-plugin could add this to the site, like is done for other [attributes]
>
> It does: https://github.com/apache/maven-plugin-tools/blob/2b4756dd67138ac85a18499fc39f31767b10fd3e/maven-plugin-tools-generators/src/main/java/org/apache/maven/tools/plugin/generator/PluginXdocGenerator.java#L316
>
> But only for the positive case it seems.
You should handle the oppositve case as well, making the "not" emphesized. Happy to review a PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-973095540
So, are we dropping this of add thi intentionally saying that we have analyzed and concluded that it is not threadsafe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: Fix for SCM-946 - Parallel execution (@threadSafe)
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724325874
Good question - I don't. Do you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-973302748
I think we said and concluded that the threadSafe annotation set to false doesn't remove the warning. Whether the plugin(s) are actually threadSafe is another topic imho and was not discussed here or did I miss someting?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972723395
@pzygielo Yes the warning doesn't go away since it is set to `false`. So skipping it or setting it to false (warning-wise) is the same result. It must be set to true to remove the warning. As the warning states using `@threadSafe` (without true or false) means it is true and only then the warning goes away. I mistakenly thought when setting it to false would also remove the warning since a plugin writer explicitly stated the threadsafety...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Not specifying it: **Warning printed**, should be no parallel execution (but there are certain constellations I have doubt about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation)
False = **No warning**, no parallel execution
True = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Case1: no annotation: **Warning printed**, should be no parallel execution (but there are certain constellations I have doubts about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation or a missing one)
Case2: annotation set to false = **No warning**, no parallel execution
Case3: annotation set to true = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970459864
@Lonzak agree, just would be great to not see dozens of PR to explicitly set the attribute and let external plugins benefit from it OOTB too - it is not a trivial behavior since generally annotations defaults are kept when not using this kind of bytecode parsing. But no issue with this PR for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972653112
@Lonzak - what is your maven version?
I still can't see it being fixed by [current changes](85b640113bf9e943534c0f63850bf0a5bd6942fb).
```shell
$ git tag -d v34 ; mvn -V scm:tag -DpushChanges=false -DconnectionUrl=scm:git:git://server/repo -Dtag=v34 -T4
Deleted tag 'v34' (was 0d8d2a7)
Apache Maven 3.8.3 (ff8e977a158738155dc465c6a97ffaf31982d739)
Maven home: /usr/local/apps/apache-maven-3.8.3
Java version: 11.0.13, vendor: Red Hat, Inc., runtime: /usr/lib/jvm/java-11-openjdk-11.0.13.0.8-2.fc35.x86_64
Default locale: en_GB, platform encoding: UTF-8
OS name: "linux", version: "5.14.17-301.fc35.x86_64", arch: "amd64", family: "unix"
[INFO] Scanning for projects...
[INFO]
[INFO] Using the MultiThreadedBuilder implementation with a thread count of 4
[INFO]
[INFO] ----------------------------< xavier >-----------------------------
[INFO] Building Xavier 1-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[WARNING] *****************************************************************
[WARNING] * Your build is requesting parallel execution, but project *
[WARNING] * contains the following plugin(s) that have goals not marked *
[WARNING] * as @threadSafe to support parallel building. *
[WARNING] * While this /may/ work fine, please look for plugin updates *
[WARNING] * and/or request plugins be made thread-safe. *
[WARNING] * If reporting an issue, report it against the plugin in *
[WARNING] * question, not against maven-core *
[WARNING] *****************************************************************
[WARNING] The following plugins are not marked @threadSafe in Xavier:
[WARNING] org.apache.maven.plugins:maven-scm-plugin:1.11.3-SNAPSHOT
[WARNING] Enable debug to see more precisely which goals are not marked @threadSafe.
[WARNING] *****************************************************************
[INFO]
[INFO] --- maven-scm-plugin:1.11.3-SNAPSHOT:tag (default-cli) @ xavier ---
[INFO] Final Tag Name: 'v34'
[INFO] Executing: /bin/sh -c cd '/tmp/xavier' && 'git' 'tag' '-F' '/tmp/maven-scm-1378539639.commit' 'v34'
[INFO] Working directory: /tmp/xavier
[INFO] Executing: /bin/sh -c cd '/tmp/xavier' && 'git' 'ls-files'
[INFO] Working directory: /tmp/xavier
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.867 s (Wall Clock)
[INFO] Finished at: 2021-11-18T09:41:22+01:00
[INFO] ------------------------------------------------------------------------
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970073979
But what about setting it to false if you or the project is not sure about this? And later if that is clarified change it to true when clarified? And who can say that it is thread safe? Anyone of the core developers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Not specifying it: **Warning printed**, should be no parallel execution (but there are certain constellations I have doubts about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation or a missing one)
False = **No warning**, no parallel execution
True = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on a change in pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on a change in pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#discussion_r751454226
##########
File path: maven-scm-plugin/src/main/java/org/apache/maven/scm/plugin/AbstractScmMojo.java
##########
@@ -57,6 +58,9 @@
* @author <a href="evenisse@apache.org">Emmanuel Venisse</a>
* @author Olivier Lamy
*/
+
+@Mojo( name = "ScmMojo",
Review comment:
This adds new goal, including documentation (site, `:help`)
but such goal cannot be executed:
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-scm-plugin:1.11.3-SNAPSHOT:ScmMojo (default-cli) on project xavier: Execution default-cli of goal org.apache.maven.plugins:maven-scm-plugin:1.11.3-SNAPSHOT:ScmMojo failed: Unable to load the mojo 'ScmMojo' (or one of its required components) from the plugin 'org.apache.maven.plugins:maven-scm-plugin:1.11.3-SNAPSHOT': com.google.inject.ProvisionException: Unable to provision, see the following errors:
[ERROR]
[ERROR] 1) No implementation for org.apache.maven.scm.plugin.AbstractScmMojo was bound.
[ERROR] while locating org.apache.maven.scm.plugin.AbstractScmMojo
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972947699
😞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Not specifying it: **Warning printed**, should be no parallel execution (but there are certain constellations I have doubt about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation or a missing one)
False = **No warning**, no parallel execution
True = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Not specifying it: **Warning printed**, should be no parallel execution (but I there are certain constellations I have doubt about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation)
False = **No warning**, no parallel execution
True = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971468083
@rmannibucau In an ideal world I would like to have support for case3 (cp. above) which can be called in parallel. But as I understand that is currently not possible since nobody can really say whether it is thread-safe or not. So I suggested to go with case2 to have a defined behavior as you called it - meaning to clearly indicate that the plugin is not threads-safe (and thus to avoid any misbehaving). The current status is case1 which in my opinion might cause problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o closed pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #107:
URL: https://github.com/apache/maven-scm/pull/107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: Fix for SCM-946 - Parallel execution (@threadSafe)
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724350785
Why did you mark it as such?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: Fix for SCM-946 - Parallel execution (@threadSafe)
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724286412
How do you know that this is thread safe?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971673991
Same here - I didn't get your last part. I'll try to explain it again:
I would like to have the threadSafe annotation defined (true or false) to avoid any misbehavior.
1. Case: No threadSafe annotation specified - current situation today
2. Case: ThreadSafe Annotation=false - situation after my fix
3. Case: ThreadSafe Annoation = true - situation when threadSafety has been ensured (someday in the future)
For me:
Case 1 = sequential execution but in certain cases it was **misbehaving** for me when using `-T` (+ warning is printed) => ambiguous
Case 2 = always sequential execution (and no warning)
Case 3 = parallel execution when `-T` has been specified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-973305961
> I think we said and concluded that the threadSafe annotation set to false doesn't remove the warning. Whether the plugin(s) are actually threadSafe is another topic imho and was not discussed here or did I miss someting?
Correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971879439
I don't understand how this shall help, as `plugin.xml` _already_ has following:
```xml
<mojo>
<goal>tag</goal>
...
<threadSafe>false</threadSafe>
<parameters>
...
```
As there is no reproducer I used mine, which might be wrong, and this change for `scm:tag` still gives me
```
[WARNING] *****************************************************************
[WARNING] * Your build is requesting parallel execution, but project *
[WARNING] * contains the following plugin(s) that have goals not marked *
[WARNING] * as @threadSafe to support parallel building. *
[WARNING] * While this /may/ work fine, please look for plugin updates *
[WARNING] * and/or request plugins be made thread-safe. *
[WARNING] * If reporting an issue, report it against the plugin in *
[WARNING] * question, not against maven-core *
[WARNING] *****************************************************************
[WARNING] The following plugins are not marked @threadSafe in Xavier:
[WARNING] org.apache.maven.plugins:maven-scm-plugin:1.11.3-SNAPSHOT
[WARNING] Enable debug to see more precisely which goals are not marked @threadSafe.
[WARNING] *****************************************************************
```
thus I can't confirm https://github.com/apache/maven-scm/pull/107#issuecomment-971673991.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] pzygielo commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
pzygielo commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972711956
I guess in runtime just `plugin.xml` is used, as [`@Mojo`](https://github.com/apache/maven-plugin-tools/blob/a6e2a968b1e760328662ac7a0a71df8b15d29803/maven-plugin-annotations/src/main/java/org/apache/maven/plugins/annotations/Mojo.java#L36) is [`RetentionPolicy.CLASS`](https://docs.oracle.com/javase/7/docs/api/java/lang/annotation/RetentionPolicy.html#CLASS) only.
And changes presented here have no impact on `plugin.xml` produced for scm plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-972895839
So, basically the outcome of this is a noop?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] rmannibucau commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971362262
@Lonzak goal would be to be defensive by default, ie make it work even if you call -T but in degraded mode (not really parallel at the execution), think it is better than a warning nobody cares about - it is the case right?. I'm fine always having a warning if I run a parallel build but the build is not parallel friendly, think it is saner than misbehaving and logging something by default, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970061049
No changes so far because on really needs to know that the code is truely threadsafe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: Fix for SCM-946 - Parallel execution (@threadSafe)
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-724325874
Good question - I don't.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974686904
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970183541
>
>
> Shouldn't MojoAnnotationVisitor be changed to ensure there is a safe default to avoid such an issue (instead of just relying on visited value, rely on spec - reflection - + visited value as overrides for values with defaults)?
Sounds reasonable...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970131225
>
>
> There is a big difference: when you set it to false your plugin would not be executed in parallel and the warning would not appear. I mean in case of doubt set it to false - now the behavior is undefined and the plugin thus might even be executed in parallel which could lead to failures. So explicitly set it (even if it is false)
This, I did not know. Thanks. Please prepare/update a PR with `false` and I will add it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-970411568
Ok I changed it to false.
@michael-o If you mean the [MojoAnnotationVisitor ](https://maven.apache.org/components/plugin-tools-archives/plugin-tools-3.0/apidocs/org/apache/maven/tools/plugin/annotations/scanner/visitors/MojoAnnotationVisitor.html) it certainly would make sense define some reasonable default or fallback value. But isn't this another maven project? (I am not so familiar with the overall project structure...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971342688
I don't think it is a noop even though the default is false.
> threadSafe - @threadSafe <true\|false> - Default: false
> Marks this mojo as being thread-safe, i.e. the mojo safely supports concurrent execution during parallel builds. **Mojos without this annotation will make Maven output a warning when used during a parallel build session.** The short-hand notation @threadSafe without a tag value is equivalent to @threadSafe true.
So when you specify the tag and set it to false there will be **no** warning. Maven and developers know that the plugin is not thread safe. Basically:
Case1: Not specifying it: **Warning printed**, should be no parallel execution (but there are certain constellations I have doubts about this since we had problems before. But might have been a bug in maven itself interpreting the threadSafe annotation or a missing one)
Case2: False = **No warning**, no parallel execution
Case3: True = **No warning**, parallel execution
Thus it doesn't look like a noop to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak edited a comment on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak edited a comment on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971673991
Same here - I didn't get your last part. I'll try to explain it again:
I would like to have the threadSafe annotation defined (true or false) to avoid any misbehavior.
1. Case: No threadSafe annotation specified - current situation today
2. Case: ThreadSafe Annotation=false - situation after my fix
3. Case: ThreadSafe Annoation = true - situation when threadSafety has been ensured (someday in the future)
For me:
Case 1 = sequential execution but in certain cases it was **misbehaving** for me (+ warning is printed) => ambiguous
Case 2 = always sequential execution (and no warning)
Case 3 = parallel execution when `-T` has been specified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] Lonzak commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
Lonzak commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-971673991
Same here - I didn't get your last part. I'll try to explain it again:
I would like to have the threadSafe annotation defined (true or false) to avoid any misbehavior.
1. Case: No threadSafe annotation specified - current situation today
2. Case: ThreadSafe Annotation=false - situation after my fix
3. Case: ThreadSafe Annoation = true - situation when threadSafety has been ensured
For me:
Case 1 = sequential execution but in certain cases it was **misbehaving** for me (+ warning is printed) => ambiguous
Case 2 = always sequential execution (and no warning)
Case 3 = parallel execution when `-T` has been specified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #107: [SCM-946] Added @threadSafe for parallel execution
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #107:
URL: https://github.com/apache/maven-scm/pull/107#issuecomment-974700612
> > I think we should merge this and rename the ticket to make it explicitly clear that this plugin is _not_ threadsafe.
>
> Goals _are_ already marked with threadSafe=false, there is no third option. It would not make it more not thread safe. It'd be _explicitly clear_ only in the code, so interested party would need to check it in the source. I don't like it. It would have no impact on the build itself.
>
> Perhaps plugin-plugin could add this to the site, like is done for other
>
> > Attributes:
> > Requires a Maven project to be executed.
> > Executes as an aggregator plugin.
>
> This would as well save from analysis and changes to _actually_ make the goals thread safe.
>
> Hence I'm 👎 for merging. (But I'm not maintainer here, so only sharing my opinion.)
Thank you very much for the analysis effort. Closing this one as it has no effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org