CI for forward-porting GC3 patches to GC4 #147

ddeclerck · 2024-05-29T22:26:35Z

Follow-up of #146.

GitMensch · 2024-05-31T18:56:18Z

build_aux/bootstrap

@@ -96,12 +96,15 @@ autoreconf $AC_OPTS $MAINPATH > $msgs 2>&1; ret=$?
 # Filter aminclude_static as those are only used _within_ another
 #        check so reporting as portability problem is only noise.
 # This has the effect of redirecting some error messages to stdout. 
+# to be moved to the Makefile - currently only usable for bootstrap,
+# but should be done on autogen, too


A, that old TODO...

GitMensch

I suggest to wrap the commits again. From what I've inspected we need one refactor for integrating 4.x logic (you've spotted that well) nicely.

GitMensch · 2024-05-31T19:02:49Z

libcob/fileio.c

-	snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "IO_", s);
-	if ((file_open_io_env = cob_get_env (file_open_env, NULL)) == NULL) {
-		snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "io_", s);
+	if (f != NULL) {


When/why should f be null here?

Because some functions (open_cbl_file, cob_sys_delete_file, ...) were "improved" to perform file mapping in GC3 (rev 3944), by calling the cob_chk_file_mapping function, which does not take a cob_file argument in GC3 but does in GC4, and that function in turn calls cob_chk_file_env. Since these functions (open_cbl_file, cob_sys_delete_file, ...) do not use a cob_file object, I resorted to passing NULL and coping with that...

Is this okay for you @GitMensch ?

libcob/fileio.c

GitMensch · 2024-05-31T19:48:51Z

oops, hope I haven't broken the gitignore in f36dcda - if not then we likely should apply that to the gcos3x branch as well.

ddeclerck · 2024-05-31T21:21:13Z

I suggest to wrap the commits again. From what I've inspected we need one refactor for integrating 4.x logic (you've spotted that well) nicely.

Saw your message a bit late, added another commit in the meantime 😅

By wrapping up you mean, committing to SVN ? (after doing the requested modifications of course)

GitMensch · 2024-06-01T07:50:07Z

This approach is reasonable in general. ... So you did already check that this piece of code changed in a later commit? If you see a difference in the current code, then you can use "svn annotate" to check which commit did the change in this party of fileio.c to know what to merge before the refactoring. :-) Am 1. Juni 2024 00:07:13 MESZ schrieb OCP David Declerck ***@***.***>:

…

@ddeclerck commented on this pull request. > + /* apply COB_FILE_PATH if set (similar to ACUCOBOL's FILE-PREFIX) */ + if (file_paths) { + for(k=0; file_paths[k] != NULL; k++) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + if (access (file_open_buff, F_OK) == 0) { + break; + } +#if defined(WITH_CISAM) || defined(WITH_DISAM) || defined(WITH_VBISAM) || defined(WITH_VISAM) + /* ISAM may append '.dat' to file name */ + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s.dat", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + if (access (file_open_buff, F_OK) == 0) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[k], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + break; + } +#endif + } + if (file_paths[k] == NULL) { + snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s", + file_paths[0], SLASH_CHAR, file_open_name); + file_open_buff[COB_FILE_MAX] = 0; + } + strncpy (file_open_name, file_open_buff, (size_t)COB_FILE_MAX); + } I remember why I haven't refactored that straight away : in case subsequent commits modify the same code, conflicts will be much easier to handle. I was thinking about keeping the refactoring for after all the patches are merged... -- Reply to this email directly or view it on GitHub: #147 (comment) You are receiving this because you commented. Message ID: ***@***.***>

ddeclerck · 2024-06-01T09:03:33Z

I tend to be overly "conservative". Indeed this piece of code is barely modified afterwards, so I'll do the refactoring.

ddeclerck · 2024-06-05T08:53:45Z

Is this okay to merge (@GitMensch) ?

GitMensch · 2024-06-05T14:37:29Z

I'll try to review this (late) evening.

GitMensch · 2024-06-06T05:38:21Z

looks_absolute should use "src", not file_open_name directly (merge issue?)

"apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit.

We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well.

ddeclerck · 2024-06-06T07:02:09Z

looks_absolute should use "src", not file_open_name directly (merge issue?)

This change is introduced in a later commit (3993).

"apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit.

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well.

By "new use", de you mean the fact that we apply file paths to the complex case ?

GitMensch · 2024-06-06T12:48:20Z

By "new use", de you mean the fact that we apply file paths to the complex case ?

yes

This change is introduced in a later commit (3993).

good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

Depends on how other functions do it - it is best for now to mimic that (once the merge is completed we may revisit that part, but there are "some" commits left until we get there).

ddeclerck · 2024-06-08T10:01:57Z

I made the necessary changes.

This change is introduced in a later commit (3993).

good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well

I find it more convenient to merge consecutive commits. If that's okay for you I could add to the current batch the next eligible commits until 3993 (that would be 6 commits: 3973, 3979, 3988, 3989, 3992 and 3993).

GitMensch · 2024-06-08T10:52:34Z

That batch is good to go :-)

ddeclerck · 2024-06-08T11:14:22Z

That batch is good to go :-)

Merged in SVN ;)

I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ?

GitMensch · 2024-06-08T11:38:48Z

I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ?

No, only new files are copied, the others left as-is; before a release I regenerate the files but the files are nearly completely maintained by the translation project.
So just record the merge, then copy over new files and svn add them, if there are any.

And of course

ddeclerck · 2024-06-08T13:54:13Z

Alright.

And of course

Hope this unfinished sentence did not have any vital info 😅

GitMensch · 2024-06-08T14:01:22Z

I can't remember any important info missing there.

codecov-commenter · 2024-06-08T14:17:10Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 69.42446% with 170 lines in your changes missing coverage. Please review.

Please upload report for BASE (gc4@caeacd5). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
cobc/typeck.c	69.51%	49 Missing and 26 partials ⚠️
cobc/tree.c	64.48%	11 Missing and 27 partials ⚠️
cobc/codegen.c	64.28%	27 Missing and 8 partials ⚠️
libcob/common.c	84.93%	7 Missing and 4 partials ⚠️
cobc/parser.y	54.54%	6 Missing and 4 partials ⚠️
cobc/config.c	66.66%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@          Coverage Diff           @@
##             gc4     #147   +/-   ##
======================================
  Coverage       ?   65.72%           
======================================
  Files          ?       37           
  Lines          ?    65593           
  Branches       ?    18281           
======================================
  Hits           ?    43113           
  Misses         ?    15457           
  Partials       ?     7023

Flag	Coverage Δ
	`65.72% <69.42%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ddeclerck · 2024-06-08T22:54:46Z

Quick question: I sometimes see alternative code for GC4 in #if 0 blocks; I guess I should implement those and drop the other branch, correct ?

I'm talking about those:

#if 0 /* TODO for 4.0: set the attributes from the field given outside on the stack */
		output ("cob_field *cob_fret, const int cob_pam");
#else
		output ("cob_field **cob_fret, const int cob_pam");
#endif

GitMensch · 2024-06-12T20:21:25Z

That's quite a bunch - any reason to not merge upstream?
Open issues you are aware of or special adjustments needed?

[we really need to get to commits that have someone else in the ChangeLogs...]

ddeclerck · 2024-06-12T21:35:57Z

That's quite a bunch - any reason to not merge upstream? Open issues you are aware of or special adjustments needed?

No good reason. It may be many commits, but the first batch had way more lines (this one is only +1,162 −904).
Anyways, I'll merge upstream if that's okay with you.

[we really need to get to commits that have someone else in the ChangeLogs...]

I'm looking forward to reaching commit 4614 - our first contribution to GC3 ;)

ddeclerck · 2024-11-18T13:02:54Z

this bunch is ready for commit (but as we did adjust P-handling here I'd rather see a test in 3.x and the merge of that here before it gets checked in)

Commits 4998 and 5018 from 3.x already bring various tests related to P, and I also merged in advance the "binary size with PIC P" tests from the yet-to-be-merged PR #197 .
What additional tests would you want ?

GitMensch · 2024-11-18T13:14:39Z

This TODO is about using those P fields (both to left and right of the 9) in a JSON/XML GENERATE. They should be output as in their pretty-display variant.

ddeclerck · 2024-11-18T14:30:59Z

This TODO is about using those P fields (both to left and right of the 9) in a JSON/XML GENERATE. They should be output as in their pretty-display variant.

Hadn't realized you were talking about those.

Would this work ? #198

GitMensch · 2024-11-18T14:50:56Z

Yes, that's fine - if this works locally as it does in GC3, then feel free to merge the other to 3.x and this to trunk (a later merge will include this one [note: I suggest to not include the to-be-commited-by-me tests into trunk now, as this will ease the merge of that 3.x commit later on - which should not be too far away in any case).

ddeclerck · 2024-11-18T14:54:01Z

note: I suggest to not include the to-be-commited-by-me tests into trunk now, as this will ease the merge of that 3.x commit later on - which should not be too far away in any case

Noted, I'll remove them from here.

ddeclerck · 2024-11-19T09:54:05Z

@GitMensch I guess I'll stop for this batch - again not many commits but many lines (tell me if it's too much, I'll just revert the 3-4 last ones).
Everything was mostly merged without conflicts.

GitMensch

I haven't inspected data_packed.at

5,069 additions, 5,066 deletions not shown because the diff is too large. Please use a local Git client to view these changes.

But other than that this bunch looks good.
The minimal amount of conflicts is kind of expected here as there were a lot of screenio.c changes - a file nearly untouched in trunk.

Because of this someday(TM) someone(R) should run make checkmanual to verify the state - but we can postpone this to mid/end December as well.

GitMensch · 2024-11-19T14:16:16Z

This bunch can be checked in again.

ddeclerck · 2024-11-19T15:04:22Z

I haven't inspected data_packed.at

5,069 additions, 5,066 deletions not shown because the diff is too large. Please use a local Git client to view these changes.

Yeah, all those changes to this single file are brought by a single commit, 5035 - but 99 % of those changes are just whitespace changes anyways.

But other than that this bunch looks good. The minimal amount of conflicts is kind of expected here as there were a lot of screenio.c changes - a file nearly untouched in trunk.

Because of this someday(TM) someone(R) should run make checkmanual to verify the state - but we can postpone this to mid/end December as well.

Wasn't aware of this. I gave it a shot. The only failures I have:

 58: CURSOR clause                                   FAILED (run_manual_screen.at:3084)
 59: CRT STATUS clause                               FAILED (run_manual_screen.at:3193)

They just fail without even asking for anything. In the log, I can see this is because the compilation messages warning: 'FILLER' has FROM, TO or USING without PIC; PIC will be implied have now disappeared. Which is weird because those were never present in this test file in the 3.x branch, and were only added to 4.x by 4180.

Also, I couldn't check those two because my function keys have specific mappings I'd rather not touch:

 61: CRT STATUS clause                               FAILED (run_manual_screen.at:3401)
 62: X/Open CRT STATUS clause                        FAILED (run_manual_screen.at:3501)

GitMensch · 2024-11-19T15:49:18Z

Thanks for checking, concerning those two failures we should inspect them later (TODO file); RUN_PROG_MANUAL should have no compile messages at all...

as noted: bunch is good to merge

ddeclerck · 2024-11-21T11:22:28Z

I think I'll stop here for this batch. One commit brought a huge new test to the testsuite, but other than this, I believe the diff is not too big (it's many small commits). Again, most commits were merged without conflicts.

Two things that require attention though:

commit 5067 brings a new test ("DISPLAY: ADD and SUBTRACT w/o SIZE ERROR"), which fails because of optimizations only found in the 4.x version of cb_build_optim_sub ; I #if0'ed those for now (see 2d0a54f)
commit 5070 brings a new test (EXTFH: reading two files with one FCD), which fails because the output of PIC X COMP-X occupies 3 characters in 4.x while it only occupies 2 in 3.x ; as I'm not sure which is the proper behavior I adjusted the ouput (added a leading 0) for now (see 8f327f9)

GitMensch · 2024-11-25T19:36:42Z

because the output of PIC X COMP-X occupies 3 characters in 4.x while it only occupies 2 in 3.x ; as I'm not sure which is the proper behavior I adjusted the ouput (added a leading 0) for now

I'd have expected a three digit output and checking with MF this is also the result there... if you happen to see the reason for the two-digit display please apply a fix for 3.x :-)

Note - the testcase should be adjusted in any case because:

it is an error to use FCD-BINARY in all cases but when FCD-STATUS-KEY-1 = 9 - otherwise FCD-STATUS-KEY-2 should be used (which is single-char 0)
this test is not about COMP-X display, so dropping this part makes the test more focused (and if we use the approach as above, then this brings this into the test as well)

for commit 5067 - we should inspect this more ... [2 hours checking and debugging later...]:

the current expected test result is indeed fine
the issue is that the integer optimizations have no option to store a negative or positive zero - which would be needed for both signed COMP-3 and USAGE DISPLAY (there: only positive zero)
the first #if 0 is only necessary because cb_is_integer_field() does not what it say (but given the additional check for binary_truncate the calling code still expect it to) - to fix that all callers of this function have to be checked, and likely the function be split to have the non-binary integer parts be in a function like cb_is_integer_receivable_field or similar
the second #if 0 should be restored, and the following be placed before (leads to the test running again):

		/* we may want negative/positive zero,
		   which prevents a direct integer calculation */
		if ((f->usage == CB_USAGE_PACKED || f->usage == CB_USAGE_DISPLAY)
		 && (f->pic && f->pic->have_sign)) {
			return CB_BUILD_FUNCALL_3 ("cob_sub_int", v,
						   cb_build_cast_int (n), cb_int0);
		}

those adjustments have to be also done to cb_build_optim_add and a test scenario for ADD to the BCD and DISPLAY tests that triggers this before the adjustment is made
the TODO file likely needs two new entries:
- check use of new integer optimization in cb_build_optim_sub and cb_build_optim_add - those may be slower than cob_add_packed_int/cob_add_packed_int64
- if there's a reasonable performance benefit for the integer optimizations for BCD/DISPLAY: add an option -funsigned-zero which never stores a sign in those (or in one of those, depending on a perf stat result) to provide the option to still use this optimization

ddeclerck added the DO NOT MERGE label May 29, 2024

ddeclerck changed the title ~~Dummy commit~~ CI for forward-porting GC3 patches to GC4 May 29, 2024

ddeclerck force-pushed the gc4 branch from dc9dda1 to b9778c3 Compare May 30, 2024 17:29

ddeclerck force-pushed the gc3_to_gc4 branch 3 times, most recently from 6900d8d to 1c95bd0 Compare May 30, 2024 21:21

GitMensch reviewed May 31, 2024

View reviewed changes

GitMensch requested changes May 31, 2024

View reviewed changes

ddeclerck force-pushed the gc3_to_gc4 branch from c2829b1 to 18626fb Compare May 31, 2024 20:45

ddeclerck force-pushed the gc3_to_gc4 branch from 05442df to 5174bb3 Compare June 8, 2024 09:39

ddeclerck force-pushed the gc3_to_gc4 branch from 5174bb3 to e31913b Compare June 8, 2024 14:14

ddeclerck force-pushed the gc3_to_gc4 branch 2 times, most recently from 33d7596 to 01b3b1e Compare November 18, 2024 17:40

GitMensch reviewed Nov 19, 2024

View reviewed changes

Merge SVN 5050

c3dbe2d

ddeclerck force-pushed the gc3_to_gc4 branch from 497a019 to c3dbe2d Compare November 20, 2024 08:57

ddeclerck added 14 commits November 20, 2024 11:23

Merge SVN 5052

e7a20fc

Merge SVN 5053

d89f987

Merge SVN 5054, 5055, 5056

4c79976

Merge SVN 5057, 5058, 5059, 5062, 5063

095b747

Merge SVN 5060, 5061, 5064

4dd8c84

Merge SVN 5065, 5066

330a1c7

Merge SVN 5067

3bd6942

Disable some SUB optimization that breaks FLD0370B in data_display.at

2d0a54f

Merge SVN 5068

12941bf

Merge SVN 5069

18fab15

Merge SVN 5070

d6c7164

Adjust output on test introduced by 5070

8f327f9

Merge SVN 5071

fa64d49

Merge SVN 5072

c5d7f55

CI for forward-porting GC3 patches to GC4 #147

Are you sure you want to change the base?

CI for forward-porting GC3 patches to GC4 #147

Conversation

ddeclerck commented May 29, 2024

GitMensch May 31, 2024

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch May 31, 2024

Choose a reason for hiding this comment

ddeclerck May 31, 2024

Choose a reason for hiding this comment

ddeclerck Jun 3, 2024

Choose a reason for hiding this comment

GitMensch commented May 31, 2024

ddeclerck commented May 31, 2024 • edited Loading

GitMensch commented Jun 1, 2024 via email

ddeclerck commented Jun 1, 2024

ddeclerck commented Jun 5, 2024

GitMensch commented Jun 5, 2024

GitMensch commented Jun 6, 2024

ddeclerck commented Jun 6, 2024

GitMensch commented Jun 6, 2024

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 8, 2024

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 8, 2024

ddeclerck commented Jun 8, 2024 • edited Loading

GitMensch commented Jun 8, 2024

codecov-commenter commented Jun 8, 2024 • edited Loading

Codecov Report

ddeclerck commented Jun 8, 2024

GitMensch commented Jun 12, 2024

ddeclerck commented Jun 12, 2024 • edited by GitMensch Loading

ddeclerck commented Nov 18, 2024

GitMensch commented Nov 18, 2024

ddeclerck commented Nov 18, 2024

GitMensch commented Nov 18, 2024

ddeclerck commented Nov 18, 2024

ddeclerck commented Nov 19, 2024

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch commented Nov 19, 2024

ddeclerck commented Nov 19, 2024

GitMensch commented Nov 19, 2024

ddeclerck commented Nov 21, 2024

GitMensch commented Nov 25, 2024 • edited Loading

ddeclerck commented May 31, 2024 •

edited

Loading

ddeclerck commented Jun 8, 2024 •

edited

Loading

codecov-commenter commented Jun 8, 2024 •

edited

Loading

ddeclerck commented Jun 12, 2024 •

edited by GitMensch

Loading

GitMensch commented Nov 25, 2024 •

edited

Loading