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

Reproducer for #33 Maven mojo change ignored #34

Closed
wants to merge 2 commits into from

Conversation

ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Jun 28, 2020

No description provided.

@ppalaga ppalaga mentioned this pull request Jun 28, 2020
@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 28, 2020

Strange that it passed on Mac.

@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 28, 2020

@gnodet would you mind checking locally what is going on Mac? Is it really passing or the Mac CI is a false positive?

@gnodet
Copy link
Contributor

gnodet commented Jun 28, 2020

@gnodet would you mind checking locally what is going on Mac? Is it really passing or the Mac CI is a false positive?

The test succeeds on my Macbook Pro...

@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 28, 2020

The test succeeds on my Macbook Pro...

Thanks for checking! That's even stranger.

I have a PoC how to address this, at least on Linux. The idea is to remove all realms from the classworld that contain -SNAPSHOT URLs or which are under the basedir of the currently built project. I am also throwing the plexus container and all its dependents away after each build. Let me polish it a bit and amend this PR.

@ppalaga ppalaga marked this pull request as ready for review June 29, 2020 08:20
Comment on lines +283 to +284
container.dispose();
purgeRealms();
Copy link
Contributor Author

@ppalaga ppalaga Jun 29, 2020

Choose a reason for hiding this comment

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

This is the main idea of the fix: we do not re-use the container (an all its dependents) across mvnd executions and we remove -SNAPSHOTS and in-source-tree jars from the class world.

The fact that we do not ditch the whole classworld is intended to keep as many classes as possible loaded and warm. We throw away only those class loaders that are known to be mutable. We still assume that the rest of the jars in the local Maven repo is immutable. #26 is an example of a situation where it does not hold and we should have some checks in place to prevent it.

I was pondering whether we could keep parts of the plexus container across mvnd executions too (to speedup the repeated builds). It seems quite risky. I am not sure we would be able to reliably remove only all the stale beans and their dependents. The beans can be linked in uncontrollable ways. I have not (quickly) found a way how to do that. So to be safe I chose to throw the whole Plexus container.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that supposed to be done by the CliPluginRealmCache already ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually yes, see #37

BTW, I have not see CliPluginRealmCache to be picked. I had to add an explicit binding https://github.com/mvndaemon/mvnd/pull/37/files#diff-085c748cd75673826a31723d0840b82eR486

@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 29, 2020

Oh, I have a theory why the test is passing on Mac: BasicFileAttributes.fileKey() probably works differently on Mac than on Linux and windows. It looks fixing the Plugin Cache implementation would be less intrusive than 51f06c7 https://github.com/mvndaemon/mvnd/blob/master/daemon/src/main/java/org/jboss/fuse/mvnd/plugin/CliPluginRealmCache.java#L166

@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 29, 2020

On Mac, BasicFileAttributes.fileKey() seems to change when the content of the file changes https://stackoverflow.com/questions/20676800/inode-value-changed-when-editing-the-file-on-mac-os-x
That's not the case on Linux, where it is constant (device ID and inode).
I see reports that BasicFileAttributes.fileKey() returns null on Windows https://stackoverflow.com/questions/22548076/java-unique-file-id-filekey-path-filekey-ok-filekey-path-possible#comment34317450_22548076

@ppalaga ppalaga marked this pull request as draft June 29, 2020 14:09
@gnodet
Copy link
Contributor

gnodet commented Jun 29, 2020

Oh, I have a theory why the test is passing on Mac: BasicFileAttributes.fileKey() probably works differently on Mac than on Linux and windows. It looks fixing the Plugin Cache implementation would be less intrusive than 51f06c7 https://github.com/mvndaemon/mvnd/blob/master/daemon/src/main/java/org/jboss/fuse/mvnd/plugin/CliPluginRealmCache.java#L166

Ah, that would explain why it works on Mac, it's because that's the OS I developped the CliPluginRealmCache on...

@ppalaga
Copy link
Contributor Author

ppalaga commented Jun 29, 2020

Replaced by #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants