Let's talk about the test/build environment

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Let's talk about the test/build environment

Post by openBrain »

wmayer wrote: Wed Oct 05, 2022 12:30 pm Well, my idea was to merge the PR and then clean the cache manually. Then committing a new change triggers a rebuild from scratch and creates a new and hopefully valid cache.
I guess you can first delete the faulty cache (caches ?) then merge. Your merge in the master shall trigger the workflow again and we'll see. :)

EDIT : there are some mystery in how the cache action works. For PR, it takes the latest cache of the same PR (if exists) and not the latest at all, which isn't what should happen according doc...
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Let's talk about the test/build environment

Post by adrianinsaval »

I think cache for PRs are isolated from each other but the PR cache should have access to the master branch cache.
btw I don't see the point in using the date for the cache name, why not just the run_number? The cache is supposed to use the latest matching cache anyways so having the date in the name doesn't seem very meaningful.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Let's talk about the test/build environment

Post by openBrain »

mlampert wrote: Mon Oct 03, 2022 7:08 am Anything else I should or can try?
wmayer wrote: Wed Oct 05, 2022 12:30 pm Well, my idea was to merge the PR and then clean the cache manually. Then committing a new change triggers a rebuild from scratch and creates a new and hopefully valid cache.
OK, finally I had to let all this cool a bit down but now I got it (finally). :)
There are no link to cache, so you may ignore my previous deductions (sorry for the noise).
Actually, the problem is this line of the CI : https://github.com/FreeCAD/FreeCAD/blob ... s.yml#L231

It's done to harmonize line-ending. It doesn't modify the code stored in GH, but only the local code of the runner. I guess the goal is that Ccache get the same hash for a file even if one modified the line endings.
Unfortunately, this step has been implemented in a very harmful way :
* It runs on all changed files, whatever type or content it is
* It's very brutal, as it just removes '\r' from the file, even ignoring if it's at the end of a line
As you may have guessed now, the problem is that it did it on the 'test_filenaming.fcstd' file, which is a ZIP (so binary) file that (it happened here) can contain '\r' character for absolutely any reason but meaning a line end.
It so breaks the file structure and obviously prevent FreeCAD to open it.

Last word : why may the same code succeed or fail ?
There is a flaw in the action that is used to determine changed files : https://github.com/FreeCAD/FreeCAD/blob ... s.yml#L208
Actually it returns files changed by latest commit only.
So in our case, if 'test_filenaming.fcstd' changes were part of the last commit, CI worklow fails. But just add another commit (as I did to disable cache) and there it succeeds.

I'm glad to finally get the root cause and I submitted a minimal PR now that just disable the faulty step (which is absolutely not needed) : https://github.com/FreeCAD/FreeCAD/pull/7564

I plan to work soon on the CI workflow to make it easier to understand and maintain, and fix different flaws I already saw in it.

EDIT : if you want to see the failure, you can run the linked 'sed' command on the 'test_filenaming.fcstd' file and test yourself. :)
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Let's talk about the test/build environment

Post by openBrain »

@mlampert @chennes just merged the CI fix. Can you rebase and push (hopefully last time) ?
User avatar
chennes
Veteran
Posts: 3881
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Let's talk about the test/build environment

Post by chennes »

Merged: git commit 1793014dd

Thanks for the investigation and detailed explanation of what's going wrong. Do you plan on re-writing the line ending code to be less "brutal"? Or should we just remove it entirely?
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Let's talk about the test/build environment

Post by openBrain »

chennes wrote: Thu Oct 06, 2022 1:08 pm Thanks for the investigation and detailed explanation of what's going wrong. Do you plan on re-writing the line ending code to be less "brutal"? Or should we just remove it entirely?
I need some time to think about it. I guess at the end we shall define a rule, then just analyze and report faulty files.
The only purpose of really changing it (locally in workfow) is that Ccache will get the correct hash if a file has different line endings. But this is unlikely to happen, as probably if one changed it, it's probably because he/she also changed code (so cache is useless). ;)
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Let's talk about the test/build environment

Post by wmayer »

Well done!

And many thanks for the detailed explanation!
EDIT : if you want to see the failure, you can run the linked 'sed' command on the 'test_filenaming.fcstd' file and test yourself
Good anticipation :D
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Let's talk about the test/build environment

Post by adrianinsaval »

What's the point in harmonizing line endings for CI?
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Let's talk about the test/build environment

Post by openBrain »

adrianinsaval wrote: Thu Oct 06, 2022 4:42 pm What's the point in harmonizing line endings for CI?
I explained (twice) what I think is the reason. ;) But it looks like something that we can well live without. ;)
mlampert
Veteran
Posts: 1772
Joined: Fri Sep 16, 2016 9:28 pm

Re: Let's talk about the test/build environment

Post by mlampert »

openBrain wrote: Thu Oct 06, 2022 1:08 pm @mlampert @chennes just merged the CI fix. Can you rebase and push (hopefully last time) ?
I will, should be at home in an hour or two and gladly rebase and push again.
Thanks for tracking this down - that's quite the story!
Post Reply