TEST to look at Gh build vs tip #7247

karianna · 2024-12-24T04:12:04Z

No description provided.

Added some logging.

Removed explicit System.exit(0) in Main. It allows to enable "slowtest" that generates and compares XMLs. Marked SystemExitInterceptor as deprecated and removal=true. Since Java does not offer any alternative, we have to find a better solution. P.S. NSIS is planned to be removed to use jpackage only.

…s StackTrace to determine the logger name. This is expensive and replaced with an explicit logger creation.

Vest · 2024-12-24T08:13:44Z

Please wait (as usual, I cannot tell until when) and don't merge this PR

…Gen#7056

…methods that return null. In some cases, the null reference/result was not checked, but the code never failed, because it was not executed. There were no tests, that is why it was difficult to check, whether the refactoring was right :(

Update gh_build

1. Corrected the typo in special properties (light crossbow); 2. Corrected maxdex for MAXDEX bonus;

karianna · 2024-12-27T23:09:30Z

Yeah I assume we won't merge this PR, I figured it was just the easiest way to look at a diff and see if we could spot anything.

# Conflicts: # data/pathfinder/paizo/roleplaying_game/core_rulebook/cr_equip_arms_armor.lst

karianna · 2025-01-07T23:09:45Z

Super interesting those tests fail for you but not for me locally!

Vest · 2025-01-08T09:44:45Z

If you use your version/branch, but not mine, I bet you have this PR: #6975
It comments the most important part, when the character sheet is exported. I have uncommented it, and now the tests started failing. (mostly because LegacyKing corrected some data-files).

karianna · 2025-01-11T06:46:13Z

If you use your version/branch, but not mine, I bet you have this PR: #6975 It comments the most important part, when the character sheet is exported. I have uncommented it, and now the tests started failing. (mostly because LegacyKing corrected some data-files).

OK, so just a data fix required then?

…e the difference between "golden" and generated files). Trimmed lines in base.xml and base-xml.ftl template.

Corrected pf_Rogue.xml result: +1 reflex comes from Halfling Luch and +3 from his Cloak.

Vest · 2025-01-11T11:53:07Z

Not only:

DATA changes caused slowtests to fail
Some XML files were not trimmed
The template was trimmed (no leading whitespaces)
That's all. I am waiting for the next build 😄

…actions" Created a "graceful exit" object + interceptor. It is supposed to replace System.Exit calls in different parts of the project; Updated reporting.gradle by increasing versions of some dependencies; Removed a DM_EXIT rule from spotbugs -> the calls of System.Exit must be replaced with GracefulExit. The decision to create a custom interceptor was taken based on the discussion from Reddit: https://www.reddit.com/r/java/comments/1fpxmfp/jep_486_permanently_disable_the_security_manager/

Vest · 2025-01-11T16:53:16Z

Two commits above this comment are related to #7079. Security manager is mostly used to intercept System.exit calls;
I have created required classes to replace System.exit with my proposed class.

I hope, you will like it.

karianna · 2025-01-11T22:33:10Z

Looks like it all passed! Shall we take out of Draft?

Vest · 2025-01-11T22:51:31Z

It is unfinished. Do you insist? I need:

remove security manager
replace all System.exit calls
I have disabled NSIS, but I have not moved to jpackage a windows build yet.

If you don’t need release builds for windows for now. I would implement my PR simply to have indicators that tests are green.

Are you sure? Because I am not.

You can actually merge it, but I only if you plan to work with the new codebase. E.g., continue improving GH actions. If not, I would keep it for a while draft.

… DSL has been deprecated."

Replaced few System.exit with GracefulExit.exit calls. Refactored few methods to satisfy SonarQube warnings.

Refactored CommandLineArguments and its test. Increased the code coverage.

Vest · 2025-01-13T09:59:38Z

The commit 59833c5 addresses the issue #7079. We don't use SecurityManager directly (maybe its exceptions are still thrown here and there), but I have created a custom interceptor, called pcgen.util.ExitInterceptor. It is used in several test-classes.
@karianna, if you want, you can link this PR to the mentioned issue #7079
@Azhrei, I know, you were assigned to this issue. Since I haven't heard any news from you, I think, you were busy with your life. If you are curious about my implementation, can you please review this PR? Not entirely, but you can look at the class: GracefulExit, ExitInterceptor, ExitFunction and few tests that I modified.

I don't know, if I should move my three "exit"-related classes/interfaces into a nested package (just to reduce litter in utils package). If you think, it will be a good idea, I will do this. Something like: "pcgen.util.exit".

Thank you all.

Vest · 2025-01-13T10:01:53Z

…age outside GracefulExit.

Azhrei · 2025-01-13T14:59:25Z

Thanks. I actually have some time today, so yes, I'll take a look. It'll be a couple hours from now, most likely.

Since intercepting System.exit() is a no-no, I'm very curious to see how you did it!

Vest · 2025-01-13T15:02:50Z

I don’t intercept it. I have got rid of it. There is only one place, where I allow it - GracefulExit 😄
The rest is filtered out with CheckStyle. If you accidentally write System.exit, it won’t pass CheckStyle (a simple regexp within all sources - dumb, but solid). You can bypass it, if you overcome my regexp (e.g., use reflection + build the string at runtime).
thanks.

karianna · 2025-01-13T23:21:56Z

@Vest Will happily wait until you're done, thanks for tackling this!

Azhrei

Wow, big changeset. 🙂 A lot changes not directly related to GracefulExit, but I mostly skimmed those.

I like the idea of using GE as a front-end on System.exit().

It all looks good to me!

Azhrei · 2025-01-13T23:42:27Z

code/src/java/pcgen/core/PlayerCharacter.java

-		int bonus = (load == Load.MEDIUM) ? 2 : (load == Load.HEAVY) ? 1 : (load == Load.OVERLOAD) ? 0 : statBonus;
+		int bonus = switch (load)
+		{
+			case MEDIUM -> 3;


Shouldn't this be '2' instead of '3'?

Nice catch, but no :) I have found this bug, when my integration tests failed:

Max Dex must be +3

Azhrei · 2025-01-14T00:05:48Z

code/src/java/pcgen/persistence/lst/LstFileLoader.java

@@ -98,40 +96,35 @@ public static String readFromURI(URI uri) throws PersistenceLayerException
 							"The file %s uses UTF-8-BOM encoding. LST files must be UTF-8".formatted(uri));


Error message should be in a properties file...
Also, at a higher level, a file that starts with a UTF-8-BOM actually is a UTF-8 file, so this whole area might need reworking.

Nope.
I don't honestly know what to deal with the error here. I have explained you that we had Apache Commons something (maybe String commons, I don't remember exactly) to work with BOM-files. I wanted to optimize the build, and I was trying to get rid of dependencies that are not that important.

When I analyzed all LST files, I found out that we had BOM in (maybe) 10 files, not more. It was easier for me to remove BOM and write code that fails/warns the user, if he gives an unsupported file.

LegacyKing was the first one (we don't have many data-contributors), who stepped this mine first. He was disappointed with the error, and I had to write this code just for him.

My thoughts are to add a Gradle task to go through all LST (or DATA) files and scan for BOM. Either convert them to non-BOM, or fail the build or whatever.

If I don't throw an error, a wrong LST will be pushed to the repo (I don't want this). If I throw an error, probably this build will fail and somebody else will be unhappy. Maybe I should convert LST files silently, but Karianna didn't like when "gradle test" modified .properties files even when nothing was changed. Maybe I'd better write an integration test that will fail, and after this PR is ready we will be able to see in the pull request, who is trying to break the build. Maybe I will also add a task such as "checkLst" to Gradle, so it will convert broken files to good ones. And in the test exception, I will write something like "BOM detected. Test failed. Use gradle checkLst to repair".

Sounds like you've got it under control. I wasn't aware of the backstory. 👍

Azhrei · 2025-01-14T00:06:44Z

code/src/java/pcgen/persistence/lst/LstFileLoader.java

 				ShowMessageDelegate.showMessageDialog("Preferences are currently set to NOT allow\nloading of "
-					+ "sources from web links.\n" + uri + " is a web link", Constants.APPLICATION_NAME,
-					MessageType.ERROR);
+								+ "sources from web links.\n" + uri + " is a web link", Constants.APPLICATION_NAME,


Another properties file template string...?

Moved a GUI-error message to the localization file. Formatted error messages

Vest added 17 commits November 22, 2024 15:51

Added initial GH action support

4213a98

Upgraded gradle wrapper to 8.11.1.

4d6772e

Added testing to gh-action.

89188be

First attempt to log data in test. It failed in GH actions.

3d1400a

Added the logging to all tests.

2cf3d44

Added gradle cache to a github action setup-java.

0f6b522

Disabled logging, but added the test results upload.

3e474f9

Enabled "write" permissions for test results

44a4cd7

Refactored TestHelper.java

ccf45fd

Added some logging.

Updated cache-dependency-path.

78968a1

First attempt to cache build/classes and build/jre

797b24f

Corrected a typo in gradle.yml

4b2d667

Try to clear the cache and fill it with values.

38eab4b

Merge branch 'master' into gh_build

2af5e06

Performance optimization: the debugPrint (when called frequently) use…

ea32a3a

…s StackTrace to determine the logger name. This is expensive and replaced with an explicit logger creation.

Refactored Logging.java.

4effb4f

Vest added 6 commits December 24, 2024 13:04

Upgraded gradle-wrapper from 8.11.1 to 8.12

c8495ca

Remove .run/ from git repository, because it tracked by mistake in PC…

505d668

…Gen#7056

Merge pull request #2 from PCGen/master

efd9bd9

Update gh_build

Corrected pf_Cleric test;

e049370

1. Corrected the typo in special properties (light crossbow); 2. Corrected maxdex for MAXDEX bonus;

Remove "public" and "abstract" for several interfaces (redundant)

e29ad64

Merge branch 'master' into gh_build

f6466bf

# Conflicts: # data/pathfinder/paizo/roleplaying_game/core_rulebook/cr_equip_arms_armor.lst

Added more logging to PcgenFtlTestCase.java (it will help to determin…

01b97b5

…e the difference between "golden" and generated files). Trimmed lines in base.xml and base-xml.ftl template.

Trimmed different *.xml inputs.

373f2b1

Corrected pf_Rogue.xml result: +1 reflex comes from Halfling Luch and +3 from his Cloak.

Vest added 2 commits January 11, 2025 17:34

Added JavaDoc to Exit* and GracefulExit classes.

ec70b26

Vest added 6 commits January 12, 2025 10:57

Resolved warnings for Gradle 8.12: "Space-assignment syntax in Groovy…

623933e

… DSL has been deprecated."

Moved GracefulExit from test to main (util).

66cebf9

Replaced few System.exit with GracefulExit.exit calls. Refactored few methods to satisfy SonarQube warnings.

Refactored CommandLineArguments.java to avoid hidden System.exit calls.

0717621

Corrected failed int (slow) tests

4b1146e

Corrected a wrong assertion message

113a305

Removed SystemExitInterceptor.java, because it was deprecated;

59833c5

Refactored CommandLineArguments and its test. Increased the code coverage.

Vest added 3 commits January 13, 2025 13:18

Merge branch 'master' into gh_build

857ba4b

Added a checkstyle rule that prevents the project from System.exit us…

beb34d2

…age outside GracefulExit.

Use the latest version of Freemarker template engine.

98e865c

Resolved warnings from Gradle.

d299d04

Azhrei suggested changes Jan 14, 2025

View reviewed changes

Resolved comments from Github PR.

8b14c4d

Moved a GUI-error message to the localization file. Formatted error messages

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEST to look at Gh build vs tip #7247

TEST to look at Gh build vs tip #7247

karianna commented Dec 24, 2024

Vest commented Dec 24, 2024

karianna commented Dec 27, 2024 •

edited

Loading

karianna commented Jan 7, 2025

Vest commented Jan 8, 2025

karianna commented Jan 11, 2025

Vest commented Jan 11, 2025

Vest commented Jan 11, 2025

karianna commented Jan 11, 2025

Vest commented Jan 11, 2025

Vest commented Jan 13, 2025

Vest commented Jan 13, 2025

Azhrei commented Jan 13, 2025

Vest commented Jan 13, 2025

karianna commented Jan 13, 2025

Azhrei left a comment

Azhrei Jan 13, 2025

Vest Jan 14, 2025

Azhrei Jan 14, 2025

Vest Jan 14, 2025

Azhrei Jan 14, 2025

Azhrei Jan 14, 2025

Vest Jan 14, 2025

		@@ -98,40 +96,35 @@ public static String readFromURI(URI uri) throws PersistenceLayerException
		"The file %s uses UTF-8-BOM encoding. LST files must be UTF-8".formatted(uri));

TEST to look at Gh build vs tip #7247

Are you sure you want to change the base?

TEST to look at Gh build vs tip #7247

Conversation

karianna commented Dec 24, 2024

Vest commented Dec 24, 2024

karianna commented Dec 27, 2024 • edited Loading

karianna commented Jan 7, 2025

Vest commented Jan 8, 2025

karianna commented Jan 11, 2025

Vest commented Jan 11, 2025

Vest commented Jan 11, 2025

karianna commented Jan 11, 2025

Vest commented Jan 11, 2025

Vest commented Jan 13, 2025

Vest commented Jan 13, 2025

Azhrei commented Jan 13, 2025

Vest commented Jan 13, 2025

karianna commented Jan 13, 2025

Azhrei left a comment

Choose a reason for hiding this comment

Azhrei Jan 13, 2025

Choose a reason for hiding this comment

Vest Jan 14, 2025

Choose a reason for hiding this comment

Azhrei Jan 14, 2025

Choose a reason for hiding this comment

Vest Jan 14, 2025

Choose a reason for hiding this comment

Azhrei Jan 14, 2025

Choose a reason for hiding this comment

Azhrei Jan 14, 2025

Choose a reason for hiding this comment

Vest Jan 14, 2025

Choose a reason for hiding this comment

karianna commented Dec 27, 2024 •

edited

Loading