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

Implement ApplicationRestart functions #1294

Conversation

matthiasblaesing
Copy link
Member

No description provided.

assertTrue(COMUtils.SUCCEEDED(result));
} finally {
// Last resort if test succeeds partially
Kernel32.INSTANCE.UnregisterApplicationRestart();
Copy link
Member

Choose a reason for hiding this comment

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

The try block should be around after a successful RegisterApplicationRestart. Then you can combine the UnregisterApplicationRestart call and assertTrue(COMUtils.SUCCEEDED(result)); here so that any errors bubble up.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not the intention here. UnregisterApplicationRestart is only run in the finally block, so that cleanup happens in any case. The real testing happens inside the the try-block. The real test for UnregisterApplicationRestart is in 1957.

Copy link
Member

Choose a reason for hiding this comment

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

I am possibly nitpicking, so feel free to leave as is. However this is what I am thinking the issues are and generally avoid the practice of doing what you're doing:

  • if RegisterApplicationRestart succeeds in 1923, finally calls UnregisterApplicationRestart a second time, which fails silently, which seems like bad practice.
  • if RegisterApplicationRestart fails in 1923, finally calls UnregisterApplicationRestart a first time, which fails silently, which seems like bad practice.
  • if RegisterApplicationRestart succeeds in 1923, but something else fails, finally calls UnregisterApplicationRestart a first time, which may fail silently, which will leave a registered application restart without anyone noticing, which might potentially cause another subsequent test to fail.

Happy holidays!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave as is. the reasoning is, that this is a unittest not more, not less. The only intention of the finally block is to cleanup as much as possible. It is a last resort - if it works, it will clean up, if if fails, we loose nothing. The functions themselves are tested inside the try-block.

* buffer, in characters.
*
* <p>
* If the buffer is not large enough to receive the command line, the
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to copy/paste the Microsoft documentation verbatim? I haven't checked thoroughly if they publish it under a license that allows it.

Copy link
Member Author

Choose a reason for hiding this comment

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

My take on this is, that the documentation must be part of the API and I assume API is not copyrightable/this falls under the fair use assumption. I'm not a lawyer and I'm aware, that this is still under discussion in the US.

Copy link
Contributor

Choose a reason for hiding this comment

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

All the documentation on docs.microsoft.com is open-sourced here and licensed under CC-BY-4.0.

You are free to:

  • Share — copy and redistribute the material in any medium or format
  • Adapt — remix, transform, and build upon the material
  • for any purpose, even commercially.

With only the restriction:

Attribution — You must give appropriate credit, provide a link to the license, and indicate if changes were made. You may do so in any reasonable manner, but not in any way that suggests the licensor endorses you or your use.

While many of the citations from Microsoft docs (formerly MSDN) were specifically linked in the javadocs (with @see) it's not universal. To make broad API citations more clear, we could add some sort of document in the project root more clearly documenting this usage "in any reasonable manner" (e.g., ASF projects use a NOTICE file for this purpose). The attribution could be an easy one-liner.

Copy link
Member Author

Choose a reason for hiding this comment

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

JNA is not ALv2, it is dual-licensed ALv2+LGPLv2. I don't see a problem: Either the API is copyrightable and this is not fair use, then we can pretty much delete the whole platform project, or it is not copyrightable or fair use, in that case it can stay as is.

@matthiasblaesing
Copy link
Member Author

I updated the new code with references to the MSDN entries - Ok to merge?

@matthiasblaesing
Copy link
Member Author

Hearing no objections I added a note about the requirement, that the JVM must not handle errors itself and will merge on sunday.

The ant maven tasks depend on a maven version, that still uses http
to access maven central. This access was deactivated and requests fail.
To work around this problem, the dependencies are specified manually
and downloaded to the local repository. From there the dependency can
be accessed by the "normal" plugin install procedure.
This does not cover the required javac level!
- c.s.j.p.w.Kernel32#RegisterApplicationRestart
- c.s.j.p.w.Kernel32#UnregisterApplicationRestart
- c.s.j.p.w.Kernel32#GetApplicationRestartSettings
@matthiasblaesing matthiasblaesing merged commit ed11ac5 into java-native-access:master Jan 19, 2021
@matthiasblaesing matthiasblaesing deleted the application-restart branch February 25, 2024 16:36
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.

4 participants