Skip to content

Respect target type in ov_add_version_defines #22805

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.

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

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

lmielick
Copy link
Contributor

@lmielick lmielick commented Feb 13, 2024

Details:

Make ov_add_version_defines macro respect the original library type (instead of implicit assumption based on BUILD_SHARED_LIBS).
Note adding objects into a library with $<TARGET_OBJECTS:> does not work when mixing STATIC and OBJECT targets.

@lmielick lmielick requested a review from a team as a code owner February 13, 2024 07:32
@github-actions github-actions bot added category: inference OpenVINO Runtime library - Inference category: build OpenVINO cmake script / infra labels Feb 13, 2024
@ilya-lavrenov ilya-lavrenov added the ExternalIntelPR External contributor from Intel label Feb 13, 2024
@ilya-lavrenov
Copy link
Contributor

Object libraries are required for overcome limitation of Makefiles cmake generator, which does not support flags, definitions per source file and it leads to rebuild of everything. See #16729

See also https://stackoverflow.com/questions/39525528/trying-to-trigger-a-minimal-rebuild-with-cmake-and-set-source-files-properties

@lmielick
Copy link
Contributor Author

Object libraries are required for overcome limitation of Makefiles cmake generator, which does not support flags, definitions per source file and it leads to rebuild of everything. See #16729

See also https://stackoverflow.com/questions/39525528/trying-to-trigger-a-minimal-rebuild-with-cmake-and-set-source-files-properties

Interesting, didn't test it with make, but Cmake accepts this implementation.

The primary problem I hit with the original implementation is that it decides to use STATIC or OBJECT library based BUILD_SHARED_LIBS, but I must use STATIC for the plugin target in either case. target_sources(${TARGET} PRIVATE $<TARGET_OBJECTS:${TARGET}_version>) does not work if ${TARGET}_version is OBJECT and ${TARGET} is STATIC.

@ilya-lavrenov
Copy link
Contributor

ilya-lavrenov commented Feb 13, 2024

Interesting, didn't test it with make, but Cmake accepts this implementation.

Yes, it accepts. And then cmake generates Makefiles projects, which rebuild a lot lot files when product version is changed (while actually you can pull a single commit with changes in documentation)

does not work if ${TARGET}_version is OBJECT and ${TARGET} is STATIC

Maybe you can extract target type using get_target_property(target_type ${TARGET} TYPE) and make a decision based on target_type?

@github-actions github-actions bot removed the category: inference OpenVINO Runtime library - Inference label Feb 13, 2024
@lmielick lmielick changed the title Simplify ov_add_version_defines Respect target type in ov_add_version_defines Feb 13, 2024
@lmielick
Copy link
Contributor Author

Maybe you can extract target type using get_target_property(target_type ${TARGET} TYPE) and make a decision based on target_type?

Yes, thought of the same. Repurposed this PR for such solution.
Can't change the branch name though.

@ilya-lavrenov
Copy link
Contributor

build_jenkins

@ilya-lavrenov ilya-lavrenov added this to the 2024.1 milestone Feb 14, 2024
@ilya-lavrenov ilya-lavrenov self-assigned this Feb 14, 2024
@ilya-lavrenov
Copy link
Contributor

build_jenkins

@ilya-lavrenov ilya-lavrenov modified the milestones: 2024.1, 2024.0 Feb 14, 2024
@ilya-lavrenov ilya-lavrenov added this pull request to the merge queue Feb 14, 2024
Merged via the queue into openvinotoolkit:master with commit 28ff6ce Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: build OpenVINO cmake script / infra ExternalIntelPR External contributor from Intel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants