Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add extra step to Makefile config target to force CMake rescan installed packages #4305

Closed
wants to merge 1 commit into from
Closed

Add extra step to Makefile config target to force CMake rescan installed packages #4305

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ ifneq (${CLANG_FORMAT_EXCLUDES},)
endif

ifneq (${BUILD_MISSING_DEPS},)
MY_CMAKE_FLAGS += -DBUILD_MISSING_DEPS:BOOL=${BUILD_MISSING_DEPS}
MY_CMAKE_FLAGS += -DOpenImageIO_BUILD_MISSING_DEPS:STRING=all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, thanks.

Also note that since these use my new set_cache() macro, you can also set this via environment variable... and therefore, via make argument. That is,

make OpenImageIO_BUILD_MISSING_DEPS=all

works and doesn't need any special logic in the Makefile.

Maybe we should eliminate BUILD_MISSING_DEPS entirely and just use the new name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should eliminate BUILD_MISSING_DEPS entirely and just use the new name?
I agree - also nice we can reuse same argument names in both places.
I'll address that.

endif


Expand Down Expand Up @@ -236,13 +236,18 @@ profile:
# generating makefiles to build the project. For speed, it only does this when
# ${build_dir}/Makefile doesn't already exist, in which case we rely on the
# cmake generated makefiles to regenerate themselves when necessary.
# When building missing dependencies, once configuration step completes,
# rerun the configuration so that CMake rescans newly installed packages.
config:
@ (if [ ! -e ${build_dir}/${BUILDSENTINEL} ] ; then \
${CMAKE} -E make_directory ${build_dir} ; \
cd ${build_dir} ; \
${CMAKE} -DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE} \
-DCMAKE_INSTALL_PREFIX=${INSTALL_PREFIX} \
${MY_CMAKE_FLAGS} ${working_dir} ; \
if [ "${BUILD_MISSING_DEPS}" = "1" ] ; then \
${CMAKE} ${working_dir} ; \
fi ; \
Comment on lines +248 to +250
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this change, at worst it's harmless, but I'm not sure I understand why a second invocation is needed. Can you ELI5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may actually got to the bottom of the problem which makes the above change redundant.
The libutil target - (setup_oiio_util_library function) does not account for the include path to the "missing required" dependencies just built. At the build time, they are located under ./deps/dist/include). The only include path currently set there is the final install prefix location.
So adding $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/deps/dist/include> seems to address it.

fi)


Expand Down
Loading