Skip to content

Conversation

@StevenGBrown
Copy link
Member

@StevenGBrown StevenGBrown commented Feb 24, 2018

JENKINS-49262

Apply log filters to each module build, not just the top-level module set build.

Scenarios tested with Timestamper 1.8.9:

  • parallel build, Maven 3
  • aggregate build, Maven 3
  • parallel build, Maven 2
  • aggregate build, Maven 2

With the "Build modules in parallel" option selected, each module build
would previously create two output streams to the same log file: one in
hudson.model.Run.execute and another in the MavenBuild.ProxyImpl2
constructor. Opening that second output stream caused the data written
to the log file so far to be discarded.

The second output stream is unnecessary for parallel builds and so it
is no longer created.
When the "Build modules in parallel" option is selected, the configured
build wrappers are now inherited by each of the module builds.
Previously the BuildWrapper.setUp method would be called by both the
module set builds and the module builds, but the remaining BuildWrapper
methods were called during the module set build only.

Plugins such as Timestamper, AnsiColor, Build Timeout, etc will now
work.

This requires the previous commit in order for log filters (such as
Timestamper, AnsiColor, etc) to be applied throughout the entire log
file.
When the "Build modules in parallel" option is _not_ selected, log
filters (such as Timestamper, AnsiColor, etc) are now applied to each
module log. They were previously applied to the module set build only.
@StevenGBrown
Copy link
Member Author

PR ready for review. @aheritier @jglick @oleg-nenashev

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

It looks correct. Would be nice to have some unit tests, but it's up to the maintainer. Maybe better to deliver it as is taking the issue importance

@StevenGBrown
Copy link
Member Author

Seems there are no objections. Could I have this merged please?

@StevenGBrown
Copy link
Member Author

StevenGBrown commented May 27, 2018

@aheritier @jglick @olamy @oleg-nenashev @olivergondza

May I have a volunteer to merge the branch?

Changes have been approved - thanks Oleg. I've now added test coverage too.

Please do let me know if there is anything I can do to make these changes easier to review.

@jglick
Copy link
Member

jglick commented Oct 16, 2018

I do not know who, if anyone, maintains this plugin. Generally it should be considered deprecated.

Copy link
Member

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

LGTM @olamy WDYT ?
My only doubt is the impact in term of performances.
The Maven Job is already know to kill instances ....

@olamy
Copy link
Member

olamy commented Nov 29, 2018

this need some rebase

@jglick
Copy link
Member

jglick commented Nov 30, 2018

BTW as of jenkinsci/ansicolor-plugin#132 this should not be needed for coloration. (Not clear it was to begin with—there is special coloration for Maven build metadata anyway.)

@github-actions
Copy link

github-actions bot commented Oct 9, 2021

This PR is stale because it has been open 365 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Oct 9, 2021
@github-actions github-actions bot closed this Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants