Skip to content

Conversation

@dwpaley
Copy link
Collaborator

@dwpaley dwpaley commented Apr 17, 2025

No description provided.

@dwpaley dwpaley requested a review from hattne April 17, 2025 22:35
Copy link
Collaborator

@hattne hattne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me; only nitpicks from my side. Please keep in mind that I'm a total n00b as far as these actions are concerned.

I generally like to build in a separate directory to keep the source directory clean, but for this kind of one-off build that is irrelevant.


RUN cd /app/cbflib && \
cmake . && \
make -j4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cmake --build . or possibly cmake --build . --parallel may be better here: that would remove the assumption that the Unix Makefiles generator was used.

- name: install extra dependencies
run: |
sudo apt-get update
sudo apt-get install -y libtiff-dev
Copy link
Collaborator

@hattne hattne Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little surprised you need libtiff-dev, since by default CMake should build LibTIFF in-tree. Or is this a way to get the dependencies for the in-tree LibTIFF build?

Actually, I think the environment should be set up with the HDF5 and LibTIFF dependencies. Then, the build could be configured with -DCBF_WITH_HDF5:BOOL=OFF, -DCBF_WITH_PCRE:BOOL=OFF, and -DCBF_WITH_LIBTIFF:BOOL=OFF to avoid building the dependencies in-tree. After all, the purpose is to test CBFlib, not the dependencies. It would also save some CI resources.

Note that the CQRlib dependency would still be built in-tree; I guess Ubuntu has a package for it, but I'm not sure if it'll work as things currently stand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the build error if I don't add libtiff-dev:

[ 95%] Building C object CMakeFiles/tiff2cbf.dir/examples/tif_sprint.c.o
/home/runner/work/cbflib/cbflib/examples/tif_sprint.c:48:10: fatal error: tiffio.h: No such file or directory
   48 | #include <tiffio.h>
      |          ^~~~~~~~~~
compilation terminated.
make[2]: *** [CMakeFiles/tiff2cbf.dir/build.make:79: CMakeFiles/tiff2cbf.dir/examples/tif_sprint.c.o] Error 1

from the build log here: https://github.com/dials/cbflib/actions/runs/14525513797/job/40756100837 and pretty much the same result if I try the build by hand in a container. I do note that libtiff appeared to build successfully prior to hitting this error.

Will add another commit momentarily to skip building hdf5 and libtiff. When I build according to your suggestion, then ctest runs (numbers from my dubious memory) 149 tests instead of 223. Any concerns about that?

Copy link
Collaborator

@hattne hattne Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sorta-kinda looks like tiff2cbf is trying to include tiffio.h before LibTIFF has been installed. And if one has LibTIFF in the environment, the build falls back on tiffio.h from there. I'll look into this more closely tonight.

Regarding the tests: this is to be expected. If you're building LibTIFF in-tree, you will run the LibTIFF test suite in addition to the CBFlib tests (the tests cannot be disabled in LibTIFF 4.0.6, if I recall). I don't remember the precise numbers, but 149 looks like a reasonable number of tests for CBFlib alone.

- name: build
run: |
cmake .
make -j4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: I would use cmake --build . here. I have next to no experience with these actions, I'm not sure which of the make invocations is responsible for building the thing.

@yayahjb
Copy link
Collaborator

yayahjb commented Apr 22, 2025 via email

@hattne
Copy link
Collaborator

hattne commented Apr 22, 2025

@dwpaley does c56ab7a address the issues on your end?

@yayahjb If c56ab7a does indeed fix Daniel's problem, there is arguably a bug in LibTIFF (not setting the include directories properly on the tiff target), and as far as I can see it would be present even in the current code. I've made a note to check again once CBFlib dependencies can updated.

dwpaley and others added 5 commits May 1, 2025 14:07
[skip ci]

Co-authored-by: Johan Hattne <[email protected]>
[skip ci]

Co-authored-by: Johan Hattne <[email protected]>
[skip ci]

Co-authored-by: Johan Hattne <[email protected]>
(shouldn't have skipped the tests)
@dwpaley dwpaley merged commit 9508a28 into main May 1, 2025
2 checks passed
@dwpaley dwpaley deleted the docker_doc branch May 1, 2025 20:58
@hattne
Copy link
Collaborator

hattne commented Jun 19, 2025

@yayahjb If c56ab7a does indeed fix Daniel's problem, there is arguably a bug in LibTIFF (not setting the include directories properly on the tiff target), and as far as I can see it would be present even in the current code. I've made a note to check again once CBFlib dependencies can updated.

Wrong I was! The upstream LibTIFF issue was fixed in libtiff #12, released with 4.0.10. Updating to 4.7.0 as per #90 obsoletes the workaround in c56ab7a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants