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.

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

Bump com.github.spotbugs from 6.0.28 to 6.1.0 (PCGen#7254)

… coverage my tests

Vest · 2025-01-31T16:06:27Z

@karianna please check my PR: Vest#1
Do you like it?

karianna · 2025-02-01T00:38:12Z

@karianna please check my PR: Vest#1 Do you like it?

Looks great!

Signed-off-by: Vest <[email protected]>

Vest · 2025-02-01T21:45:28Z

Ok, I'd say that we have something, that I can call "release": https://github.com/Vest/pcgen/releases/tag/v0.0.8

Attention: I am not a professional devops. So, I am experimenting with my workflow. Really. So, it can build releases now. I have tested a mac release only. The application starts. It isn't signed, but it can run.

My thoughts - delete NSIS completely. But I have to verify windows & linux builds somehow.

@karianna

Signed-off-by: Vest <[email protected]>

karianna · 2025-02-02T22:45:53Z

Ok, I'd say that we have something, that I can call "release": https://github.com/Vest/pcgen/releases/tag/v0.0.8

Attention: I am not a professional devops. So, I am experimenting with my workflow. Really. So, it can build releases now. I have tested a mac release only. The application starts. It isn't signed, but it can run.

My thoughts - delete NSIS completely. But I have to verify windows & linux builds somehow.

@karianna

Is it also possible to create .deb linux distro files? I can test on an Ubuntu system (although it'll be Arm based), I can also test Windows (Arm) but I suspect the exe file is for x86-64?

Azhrei · 2025-02-03T02:58:22Z

It is. GitHub makes a number of environments available for performing the build. x86_64 is supported for Windows and Linux, and arm64 for Linux and macOS (not sure about Windows). The Debian (including Ubuntu) and Red Hat (not sure about Fedora) Linux distros are supported.

It's not difficult to create a matrix of supported architectures and supported platforms, then let GitHub build ALL OF THEM! Obviously, you'd want to be careful about triggering that one!

gradle jpackageImage Signed-off-by: Vest <[email protected]>

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.

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

Vest added 4 commits January 14, 2025 12:03

Resolved comments from Github PR.

8b14c4d

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

Merge pull request #3 from Vest/master

198ca33

Bump com.github.spotbugs from 6.0.28 to 6.1.0 (PCGen#7254)

JaCoCo was added to github actions - now it shows the high-level code…

e06d5b4

… coverage my tests

Merge branch 'master' into gh_build

05bc739

Vest added 9 commits February 1, 2025 12:16

Create a jpackage build.

a03e8da

Signed-off-by: Vest <[email protected]>

First attempt to build releases for all OSes.

cbe681d

Signed-off-by: Vest <[email protected]>

Changed the event to build a release conditionally.

095b1ed

Signed-off-by: Vest <[email protected]>

Update gradle-release.yml

1f6a122

Corrected the artifact name

21c2248

Signed-off-by: Vest <[email protected]>

Attempt to create releases

24b463c

Signed-off-by: Vest <[email protected]>

Attempt to create releases (v2)

aebc6a4

Signed-off-by: Vest <[email protected]>

Corrected the artifact creation.

995136a

Signed-off-by: Vest <[email protected]>

Attempt to create one more release

00f09c1

Signed-off-by: Vest <[email protected]>

Updated JDK from 21.0.5 to 21.0.6

592f9e6

Signed-off-by: Vest <[email protected]>

Corrected a build for Windows (at least).

9febf0c

gradle jpackageImage Signed-off-by: Vest <[email protected]>

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 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

Vest commented Jan 31, 2025

karianna commented Feb 1, 2025

Vest commented Feb 1, 2025

karianna commented Feb 2, 2025

Azhrei commented Feb 3, 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 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

Vest commented Jan 31, 2025

karianna commented Feb 1, 2025

Vest commented Feb 1, 2025

karianna commented Feb 2, 2025

Azhrei commented Feb 3, 2025

karianna commented Dec 27, 2024 •

edited

Loading