CI for forward-porting GC3 patches to GC4

[ddeclerck]

Follow-up of #146.

[ddeclerck]

I don't get why the MacOS CI fails this way.

Indeed the "LINE SEQUENTIAL COMMIT / ROLLBACK" test was a bit tricky, as the default for COB_LS_VALIDATE changed from not set to true, so I added COB_LS_VALIDATE=0 to the command line so that it behaves "as before". Works fine under Linux & Windows...

[GitMensch]

Note: you possibly can work around that by adjusting the test to do SET ENVIRONMENT 'COB_LS_VALIDATE' TO '0' in the program

[ddeclerck]

Doesn't seem to change much (compared to having COB_LS_VALIDATE=0 in the command). Guess I'll have to get my hands on an actual Mac...

[ddeclerck]

Well, this was due to the new guard READ_WRITE_NEEDS_FLUSH around the fflush instructions in lineseq_xxx. Took me a while to figure out...

Now, how do we proceed ? Should those guards be removed ? Should we force definition of READ_WRITE_NEEDS_FLUSH on MacOS ?

[GitMensch]

I'm not sure what you mean with that - the code parts came from trunk, when merging them to 3.x I've added the READ_WRITE_NEEDS_FLUSH define as the code definitely is slower and has more syscalls / fileio when flushing often.

If the Mac problem is related to that, then the CI can just define this var, no?

When we know that this is always necessary for Darwin, then we can add this define in configure automatically - I just don't know so I didn't add it.

[ddeclerck]

I'm not sure what you mean with that - the code parts came from trunk, when merging them to 3.x I've added the READ_WRITE_NEEDS_FLUSH define as the code definitely is slower and has more syscalls / fileio when flushing often.

That's a bit tricky. Yes, the fflush comes from the trunk, and the READ_WRITE_NEEDS_FLUSH define was directly added in 3.x when merging those changes from trunk to 3.x. Now, the test that fails if you don't do fflush only exists in trunk (it's about the COMMIT/ROLLBACK feature), that's why it failed when I added READ_WRITE_NEEDS_FLUSH to trunk.

If the Mac problem is related to that, then the CI can just define this var, no?

That's what I'm doing right now, and I'm fine with leaving it that way ;)

Might be worth investigating this matter in some future though.

[GitMensch]

Hm, that sounds like there should be some fflush() in COMMIT/ROLLBACK instead, no?

[ddeclerck]

Hm, that sounds like there should be some fflush() in COMMIT/ROLLBACK instead, no?

The problem occurs in this test, but does not seem to be specifically linked to the use of COMMIT/ROLLBACK.

[GitMensch]

Please add this to the "list of things to be checked later" then.

[ddeclerck]

Maybe that's sufficient for this batch ? Only 10 commits, but the diff is pretty big already.

[GitMensch]

as noted before: current bunch is good for svn

[GitMensch]

That bunch commit led to failing CI. I didn't checked why, here's the log: https://ci.appveyor.com/api/buildjobs/552wjnxni3xaakj5/log and here the testuite failure https://ci.appveyor.com/api/buildjobs/552wjnxni3xaakj5/artifacts/testsuite.log

[GitMensch]

Note: while this is unlikely to help with the failing NIST and internal tests, I just merged two compat commits, including the common.c->libxml2/parser.,h change - sop the special handling for that in the CI need to be dropped (and it works outside of that with recent GCC versions).

... had a deeper look on the failure: it seems some part of the commit message slipped into an environment variable that is taken up by cobc. This happens from time to time, so my commit which triggered the rebuild may be enough to "fix" that. Stay tuned.

[GitMensch]

Update: new rev is built - it just was the Appveyor environment problem.

[GitMensch]

just a note: I've added another merge (build_windows, in the hope to fix an issue with Appveyor) - no need to update this repo but you may want to update your local svn checkout

... and indeed,that helped. first running appveyor VS for 4.x since months: https://ci.appveyor.com/project/GitMensch/gnucobol-trunk-vs/builds/50199212 (with artifacts, but without any tests but compilation of extras directory).

[ddeclerck]

Again, very few commits (8) but many lines changed (mostly because of stuff moved around in run_file.at). Should I stop here for this batch ?

[GitMensch]

No, please go on.

[ddeclerck]

Alright ;)

[GitMensch]

My commit is done, you may update the target branch from svn and go on any time to your liking.

Note: It should now be possible to run make distmingw in both MSYS1 and MSYS2 to create a binary package (or actually 2, one with debug symbols, one without) - this would be a good opportunity to add this to the CI definition and add those as artifacts as well.

[GitMensch]

That's a bunch of entries now, I suggest to try to finish that batch, before going with the next. I did not check the diff in detail yet, but want to ask you to - either in the batch commit or separate - inspect which of the changes in fbdb.c can/should go to flmdb.c as well (these are "very near"). Maybe a direct diff and manual merge to make them as similar as possible would be possible as well.

[ddeclerck]

That's a bunch of entries now, I suggest to try to finish that batch, before going with the next.

Yeah I was going to end this batch here ; it's quite large already (and a bit tricky at times) and the next commit to merge is rather big.

I did not check the diff in detail yet, but want to ask you to - either in the batch commit or separate - inspect which of the changes in fbdb.c can/should go to flmdb.c as well (these are "very near"). Maybe a direct diff and manual merge to make them as similar as possible would be possible as well.

I'll check that and see what I can do.

[GitMensch]

Just drop a note when you want a review of the bunch (which of course is mostly looking for some possible issues in the code, as most of the code was reviewed in 3.x already and if you catch merge issues where you wasn't sure, you'd drop a "please check X" as well).

[ddeclerck]

In fact, there was not much I could apply to flmdb.c. Biggest changes in fbdb.c where the arguments of the DB_xxx macros, which is a BDB-only stuff, as well as two or three changes regarding locks (which are not implemented in the LMDB backend anyways). I still applied the few remaining changes (mostly cosmetic), for the sake of it.

So, this is okay to review @GitMensch .

BTW, will this LMDB backend be kept ? (I seem to remember you mentionned it might be dropped)

[GitMensch]

LMDB is planned to be in the 4.0 prerelease, then we check its performance and support to think about marking it as experimental or deprecated in 4.0 final.

[GitMensch]

CI runs with multiple

attempt to reference invalid memory address (signal SIGSEGV)

so something (likely last_exception_source) needs to be fixed.

[ddeclerck]

The only global part of the merge I'm not sure about is if we still need the (enum cob_open_mode)mode cast any more.

The fact is, some (all ?) of those casts are here to remove the const qualifier that some arguments have (for instance in cob_file_open the mode argument was a const int, so I changed that to const enum cob_open_mode. Or should we istead drop the const qualifier in the argument definition ?

[GitMensch]

The only global part of the merge I'm not sure about is if we still need the (enum cob_open_mode)mode cast any more.

The fact is, some (all ?) of those casts are here to remove the const qualifier that some arguments have (for instance in cob_file_open the mode argument was a const int, so I changed that to const enum cob_open_mode. Or should we istead drop the const qualifier in the argument definition ?

I see, just wasn't sure about it. We cannot add const to the values on the left hand (obviously) as the file's mode does change. We don't want to adjust the parameter passed from the outside, so it currently is const.

As this follows the original code as well as the merged one - let's move this part to the TODO list (this is a design issue that applies to other places as well) and get on with the merge.

[ddeclerck]

I think this is it this time.

[GitMensch]

Agreed - time to push that set!

Any ETA when this is expected to be finished?

[ddeclerck]

Agreed - time to push that set!

Any ETA when this is expected to be finished?

The whole merge process ?

[GitMensch]

Sure, you possibly have an idea when the customer want that (keeping in mind that there are a bunch of TODOs after the merge) and also an idea how fast the progress may be. In any case that's an ETA, not a detailed plan with a fixed date,

[ddeclerck]

Sure, you possibly have an idea when the customer want that (keeping in mind that there are a bunch of TODOs after the merge) and also an idea how fast the progress may be. In any case that's an ETA, not a detailed plan with a fixed date,

Well, not couting the TODOs and any post-merge cleanup & issues, somewhere between September-October has been mentionned (which is indeed quite short), so they can perform some tests (swap GC3 with the GC4 we'll have at that point and see what breaks in their workflow).

[ddeclerck]

Maybe that's enough for this new batch. Don't be scared by the +8500 lines: there's a new translation file (Turkish) that adds +6500 lines all by itself. Then, even if it's 20 commits, I had almost no conflicts (or only trivial ones), so the changes should be quite solid.

[GitMensch]

I should be able to have a look at the bunch before tomorrow lunch time.

[ddeclerck]

• What happened this time with the random function and MSVC?

The problem with the RANDOM function appeared just now because I added the MSVC Debug CI (adapted from the one I made for GC3) before doing this batch - previously there was only an MSVC Release CI in the GC4 branch. Anyways, the test hangs for exactly the same reason as it fails in GC3 : a popup window by the runtime checker, complaining about a cast to a smaller type.

• Can the fileio.c formatting changes be applied to the other ones as well?

The changes in fileio.c are about the cob_sys_xxx_file functions ; I don't think that applies to other files. Unless you are referring to the changes in fextfh.c ? EDIT: did not find any specific change that would apply

[ddeclerck]

Should be good for this batch (22 commits for +2000 lines) - unless you want more.

That was mostly trivial, with very few conflicts. The only adjustments I had to do were the following:

• 4864: codegen.c:output_stmt -> had to add a case for CB_TAG_LIST at line 9197, otherwise I hit the default case (got the inspiration from 3435, where you added CB_TAG_DEBUG)

• 4865: codegen.c:emit_symtab -> added !f->flag_is_typedef to exclude TYPEDEFs from the symbol table (GC3 added this exclusion in the "old" dump code, so that seemed appropriate)

• 4875: typeck.c:validate_move -> had to add a CB_LITERAL_P check at line 11717, as the CB_CONST_P case added few lines above was not enough to rule out the problematic case (maybe the AST is different for this specific case in GC4 ?)

• 4876: codegen.c:add_field_cache -> had to add a flag_is_global parameter at line 1076 ; in GC3 that piece of code is inlined in the calling function and we already have access to the necessary information

[GitMensch]

I've checked the bunch again. The amount of commits + changes are definitely enough. They also look fine.

... "but": there is no "merge" entry from you in the ChangeLogs while it looks like you did have to do some adjustments.

RE 4865: codegen.c:emit_symtab

I'm not 100% sure if excluding the typedef is the right thing; there should definitely be no "data" pointer set (because a typedef does not have a pointer). But I'd leave this to you (after all it is an explicit exclusion so could be added in later easily).

Just to let you know: One intended use-case for the symtab is to provide - dynamically at runtime - all information that is needed for debugging (if this works out the debug extension in vscode would only read that and not need any intermediate .c files which may be out of date as well).

RE 4876: so if 3.x had that information inline before the commit - was there a refactor/other commit missing in 4.x before?

RE 4875: I think we shouldn't get there because of cb_check_move should have been called and its error return leading to the message... but then there is also a note that the parser should validate that, which is true - I suggest to leave that as is and add a new TODO entry listing that necessary change and the missing validation in the parser

[ddeclerck]

... "but": there is no "merge" entry from you in the ChangeLogs while it looks like you did have to do some adjustments.

My adjustments amount to two or three lines for each problematic case ; is it worth adding a merge entry in the changelog for such small changes ?

RE 4865: codegen.c:emit_symtab

I'm not 100% sure if excluding the typedef is the right thing; there should definitely be no "data" pointer set (because a typedef does not have a pointer). But I'd leave this to you (after all it is an explicit exclusion so could be added in later easily).

Well, the thing is, 4865 added this exclusion to output_initial_values (which is fine because we have that in GC4 as well) and in output_display_fields (old dump code from GC3 which does not exist in GC4). The first exclusion somehow prevents the typedef to have an actual storage associated, so if we don't do anything about emit_symtab, then the generated codegen is invalid (I get undefined erros about b_xx variables, presumably because there is no storage associated to these typedef items). Not sure what else I could do to the typedefs (unless I can output NULL instead of b_xx ?).

RE 4876: so if 3.x had that information inline before the commit - was there a refactor/other commit missing in 4.x before?

I think it's 4.x that extracted add_field_cache from output_base (probably because that piece of code is also used in the 4.x specific function count_all_fields). Because of this, add_field_cache does not have access to all the information we have in the output_base function. Alternatively I could just pass the actual field to add_field_cache and let it call real_field_founder - but that would compute this information twice. Or I could pass both the actual field and the field "founder" to add_field_cache...

RE 4875: I think we shouldn't get there because of cb_check_move should have been called and its error return leading to the message... but then there is also a note that the parser should validate that, which is true - I suggest to leave that as is and add a new TODO entry listing that necessary change and the missing validation in the parser

By "as-is", do you mean including my addition of CB_LITERAL_P ? (it is required to pass some test)

[ddeclerck]

(I might be able to commit that upstream this evening or tomorrow evening if this gets approved, before going offline)

[GitMensch]

As-is: yes, keep your change and put that on the TODO list.

Passing both a field and its founder is fine to do.

Outputting the data as NULL and ignore typedefs during dump (by some attribute) sound good.

Merge entry in changelog isn't needed for small adjustments as you've described (changing a function signature like for entry two above should get one, though).

[ddeclerck]

As-is: yes, keep your change and put that on the TODO list.

Done (+ "CHECKME" comment added in source code)

Passing both a field and its founder is fine to do.

Done.

Merge entry in changelog isn't needed for small adjustments as you've described (changing a function signature like for entry two above should get one, though).

I added one for the added argument.

Outputting the data as NULL and ignore typedefs during dump (by some attribute) sound good.

Done for the first part ; when you talk about ignoring typedefs during dump, you mean some change to common.c:cob_dump_symbols (possibly adding a new flag is_typedef to the structure common.h:cob_symbol) ?

[GitMensch]

Yes to the later. I haven't checked the details but would suggest an attribute like is_internal and skip those during dump.

If the field has data != NULL it is a constant, otherwise a typedef.

[ddeclerck]

Would the changes I made be okay for you @GitMensch ?

[GitMensch]

yes, fine by me

[ddeclerck]

Done for this batch.

[GitMensch]

If this doesn't create a conflict you may want to merge the testsuite adjustments (VC failure) before the other changes next.

[GitMensch]

Which parts are missing to include the configurable conversion? Would it be reasonable to review soon?

[ddeclerck]

Which parts are missing to include the configurable conversion?

I'm not sure what you're referring to by "configurable conversion" ?

Would it be reasonable to review soon?

Possibly. I'm merging 4884 (slightly tricky), you could review after that.

[ddeclerck]

Done with 4884. No big trouble, but it just took a bit of time to check everything was correct (since this commit moves quite a few code blocks around).

[ddeclerck]

I also added the very recent patch from GC3 to silence the Windows error popups under MSVC, as I belive that will help.

This is ready to review.