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 84.34066% with 57 lines in your changes missing coverage. Please review.

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

Files with missing lines	Patch %	Lines
libcob/strings.c	86.68%	16 Missing and 23 partials ⚠️
libcob/common.c	38.46%	4 Missing and 4 partials ⚠️
cobc/codegen.c	0.00%	3 Missing ⚠️
cobc/parser.y	0.00%	3 Missing ⚠️
libcob/screenio.c	57.14%	0 Missing and 3 partials ⚠️
libcob/move.c	97.36%	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       ?   66.07%           
======================================
  Files          ?       39           
  Lines          ?    68751           
  Branches       ?    19153           
======================================
  Hits           ?    45427           
  Misses         ?    16114           
  Partials       ?     7210

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 · 2025-02-14T15:37:34Z

Well, I just merged the profiling (and a few small commits, one necessary for MSVC builds), and this is quite big already (the profiling itself brings a big new file and a large test).

ddeclerck · 2025-02-14T16:10:46Z

For some reason, the "Coverage" CI suddenty started to incorrectly detect readline as being present even though not installed... I don't see how the changes made to configure.ac could explain that... For now, I'm adjusting the Coverage CI to install readline...

cobc/flag.def

GitMensch

Current bunch LGTM - put to upstream as you like.

Note: readline detection (and inclusion) may be adjusted by the use pkg_config/ncurses-config; but I agree, that this shouldn't make much difference for the coverage.

GitMensch · 2025-02-15T10:52:10Z

So we may replace that with a check for both WIN32 and CYGWIN and adjust comments (or was it Changelog) as useful.

ddeclerck · 2025-03-03T09:59:27Z

This is a bunch of many small commits, mostly cleanup and build/warning fixes (this notably improves testsuite results under MSVC).

GitMensch

minor notes found in the review

cobc/ChangeLog

cobc/replace.c

libcob/ChangeLog

GitMensch · 2025-03-03T17:06:16Z

bunch is fine for check-in

ddeclerck · 2025-03-05T17:45:36Z

And that's it for this batch.

The biggest part is Boris' contribution for thread-safety of INSPECT/STRING state structures (5302 + my fix 5406).
Note that this contribution also added backward-compatibility tests to the testsuite (a huge file : backcomp.at) - since we break compatibility with GC4, should I just remove this file ?

Other notable parts of this batch are the BDB adjustments for keys of different lengths and partial keys (5296 and 5412) and some of Chuck's modifications to moves to NUMERIC-EDITED (5287).

The rest is a few things here and there.

GitMensch

some changes requested, the biggest is the one about backward compatibilty

libcob/ChangeLog

GitMensch · 2025-03-05T19:40:02Z

libcob/fbdb.c

@@ -1418,9 +1424,14 @@ ix_bdb_open (cob_file_api *a, cob_file *f, char *filename, const enum cob_open_m
 				if (f->keys[i].tf_duplicates) {
 					p->db[i]->set_flags (p->db[i], DB_DUP);
 				}
+				/* TODO: add national compare function later */


this bunch does not include a merge to TODO, so I guess that's missing - please review the "NATIONAL support" part there and add this as a sub-item (not necessary for the first 4.x release, but something we want to inspect someday and can easily forget)

Well, there is nothing about NATIONAL support in the TODO file. Guess I'll have to add this.

Agreed - in 3.x (maybe start that locally already) after we merged that bunch to trunk. I'm looking forward to the "merge change" parts of this bunch, which will mostly if not all be about the backward compat stuff.

libcob/move.c

GitMensch · 2025-03-05T19:44:46Z

tests/ChangeLog

+	* testsuite.at: added a new test suite to test the backward compatibility
+	  of strings functions (INSPECT, STRING, UNSTRING)


that note is valid, please add a new changelog entry that says something aling "drop content as this only starts to be relevant after the first 4.0 release" and replace its contents by a m4 macro to say the same

GitMensch · 2025-03-05T19:46:44Z

libcob/strings.c

+/* Static structures for backward compatibility */
+COB_TLS struct cob_inspect_state	share_inspect_state;
+COB_TLS struct cob_string_state		share_string_state;
+COB_TLS struct cob_unstring_state	share_unstring_state;


If we still have some backward compatibility code in (is there a "new" way at alI - should the state be defined in the module?) then we need a merge change that drops the backward compatibility, going only with the new stuff. But there are no new entry points, so I guess we added this and then changed it to only be thread local in the runtime (which seems reasonable as each thread can only run one program - so that would be no problem) - it possibly could be an issue in the case of:

STRING/UNSTRING/INSPECT raises an error "in between", then goes to a USE statement which includes another of those statements, and then goes back... but as the target field is unchanged we should not depend on that state at all, should it?

If we still have some backward compatibility code in (is there a "new" way at alI - should the state be defined in the module?) then we need a merge change that drops the backward compatibility, going only with the new stuff. But there are no new entry points, so I guess we added this and then changed it to only be thread local in the runtime (which seems reasonable as each thread can only run one program - so that would be no problem)

I think the backward compatibility here was about keeping the same signature for the cob_xxx_init functions (those in turn call cob_xxx_init_internal with those share_xxx_state as first argument). I guess the intent was to replace those cob_xxx_init by cob_xxx_init_internal when we could break the API, and provide a share_xxx_state defined in the module itself.

it possibly could be an issue in the case of:

STRING/UNSTRING/INSPECT raises an error "in between", then goes to a USE statement which includes another of those statements, and then goes back... but as the target field is unchanged we should not depend on that state at all, should it?

Would you have a more specific example of such scenario ?
In any case, even if the state structures were moved to the module, this would still be a problem, right ?

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 from e266614 to 314a2a7 Compare February 14, 2025 15:32

ddeclerck force-pushed the gc3_to_gc4 branch from 5926cf4 to 5312a4b Compare February 14, 2025 16:27

GitMensch reviewed Feb 14, 2025

View reviewed changes

cobc/flag.def Outdated Show resolved Hide resolved

GitMensch reviewed Feb 14, 2025

View reviewed changes

ddeclerck force-pushed the gc3_to_gc4 branch from 5312a4b to dc450d2 Compare February 14, 2025 21:23

ddeclerck force-pushed the gc3_to_gc4 branch 5 times, most recently from 7c640cb to 9b14e76 Compare February 17, 2025 08:28

GitMensch requested changes Mar 3, 2025

View reviewed changes

cobc/ChangeLog Outdated Show resolved Hide resolved

cobc/replace.c Outdated Show resolved Hide resolved

libcob/ChangeLog Outdated Show resolved Hide resolved

Merge SVN 5287

700393e

ddeclerck force-pushed the gc3_to_gc4 branch from ea178ab to 700393e Compare March 5, 2025 14:29

ddeclerck added 9 commits March 5, 2025 15:30

Merge SVN 5289

7f8b9b8

Merge SVN 5296

cc6de83

Merge SVN 5302

12f7184

Merge SVN 5406

5937369

Fix entry position in Changelog

ecdb1f5

Merge SVN 5412

54c10ab

Adjust Changelog entry for 5412

3df4a40

Merge SVN 5304

97c1958

Merge SVN 5314

9f50ae7

GitMensch requested changes Mar 5, 2025

View reviewed changes

Empty backcomp.at

32c2886

		* testsuite.at: added a new test suite to test the backward compatibility
		of strings functions (INSPECT, STRING, UNSTRING)

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

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Feb 14, 2025

ddeclerck commented Feb 14, 2025

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch commented Feb 15, 2025 via email

ddeclerck commented Mar 3, 2025

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch commented Mar 3, 2025

ddeclerck commented Mar 5, 2025

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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