otb: include legacy vcl header file#410844
Conversation
|
During test builds of OTB on different machines, we observed a non-deterministic failure in the automatically generated file OTBMosaicHeaderTest1.cxx. The issue is caused by differing header include orders depending on the filesystem behavior (e.g., btrfs vs ext4). On some systems, the build fails with the following error: This happens because otbMosaicFunctors.h uses vcl_sqrt, which depends on the macro VNL_CONFIG_LEGACY_METHODS. This macro is defined (transitively) via: If otbStreamingStatisticsMosaicFilter.h appears before otbMosaicFunctors.h in the generated file, the build succeeds. If not, the macro is not defined when otbMosaicFunctors.h is parsed, and the build fails. This is a classic case of non-deterministic builds due to:
|
|
Thanks @qbisi !
However, the tests for: Not sure how these are handled during parallel build but seems like a cyclic dependency to me. I assume this is also one of the probable issue apart from what you have mentioned. Thanks for fixing the issue as I cannot reproduce it. |
|
Maybe you can reproduce this error on btrfs filesystem. |
|
I get following error when trying to build OTB from this PR: I get the same error when building OTB from latest master. |
|
Just tried with f29cb15 and got the same error. |
|
|
@imincik can you run |
|
|
vcl_* is defined directly in "${otb-itk}/include/ITK-5.3/vcl_legacy_aliases.h" So the final fixup is to just include "vcl_legacy_aliases.h" in "otbMosaicFunctors.h" directly. |
|
|
@qbisi , finally, it works ! Great thanks for your work. |
Hi @qbisi ! Can you please add the below to otb-itk-cmake to confirm if only adding this fixes the issue ? I'm not sure if you checked my earlier reviewed comment, but my guess is that Please check by enabling it first, if it still doesn't fix then adding the header file directly to OTB should be fine . |
|
I have tried at first priority.
|
Indeed it seems ITK has this forced to be off in the cmake. I would have preferred that we enable that in the CMake c.f. Technically we can also do a I'll open an issue upstream regarding this possibly. |
pkgs/by-name/ot/otb/package.nix
Outdated
There was a problem hiding this comment.
Hi @qbisi !
Thanks. CI seems to fails because VNL_CONFIG_LEGACY_METHODS 1 is defined at 2 places likely. It already seems to exists at itk-5.3.0/include/ITK-5.3/vnl/vnl_config.h.
May be better to just update it under ITK during build i.e. under otb-itk. Just have to update the CMakeLists.txt possibly.
May be just adding it to the otb-itk cmakeflgs should be fine:
(lib.cmakeBool "VNL_CONFIG_LEGACY_METHODS" true)
However I tried to build otb based on this branch removing your changes and still seems to build fine for me, then do we then need these changes 🤔
There was a problem hiding this comment.
This patched otb-itk will not result in a reproduceble otb build.
vcl_compiler.h will be included only once and it might be included before "otbMosaicFunctors.h" without VNL_CONFIG_LEGACY_METHODS being defined due to unpredicted sorting order of header files.
So the only solution is to just include "vcl_legacy_aliases.h" directly in "otbMosaicFunctors.h".
In fact, otb source use "vcl_legacy_aliases.h" directly in most of its test case, i guess the dev might missed this "vcl_legacy_aliases.h" in "otbMosaicFunctors.h" and they will happily adopt this change by adding "vcl_legacy_aliases.h" to "otbMosaicFunctors.h".
pkgs/by-name/ot/otb/package.nix
Outdated
There was a problem hiding this comment.
Can you please move this comment outside of this postPatch ?
|
@daspk04 do you have time/methods to contact upstream otb to adotp this patch. |
|
@imincik can we merge this pr, or contact upstream to fix this problem and fetchpatch from the commit. |
@qbisi I have reported the issue upstream, however I would suggest to add the link to the issue in the comment. However we can merge this PR first then during the next release for OTB (10.0.0) I can remove those if they are already added to upstream at that time. |
pkgs/by-name/ot/otb/package.nix
Outdated
There was a problem hiding this comment.
Hi @qbisi
Sorry may be I wasn't clear, I meant, that add the upstream issue link, so that we can keep a track of upstream issue and remove it later incase there is a fix already in upstream.
https://gitlab.orfeo-toolbox.org/orfeotoolbox/otb/-/issues/2484
|
|
Thanks @GaetanLepage ! This can be merged I believe as we have already reported the issue upstream, and the fix here already seems work for @qbisi and others (c.f1, c.f2) |
|
Hi, I am from OTB team. I currently pushed a MR in https://gitlab.orfeo-toolbox.org/orfeotoolbox/otb/-/merge_requests/1076 . When CI is ok, the changes will be merged into the "develop" branch. Thus they will be in the next OTB 10 @imincik :
If you want the latest changes (with ITK 5.3 for instance) prefer the "develop" branch. Master one is up-to-date with latest release, not latest changes. Best regards |
Thank you very much @SpaceCyclist .
I meant nixpkgs master not OTB in this comment. But thanks for OTB branch usage clarification anyway. |
@SpaceCyclist Thanks Tristan! 🙏 |
part of #407910
It's not a regression caused by updating vtk.
related hydra build succeed https://hydra.nixos.org/build/298128281
though it failed on my local build
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.