-
Notifications
You must be signed in to change notification settings - Fork 224
bazel: add release structure improvements (versioning, SONAME, vars.sh, pkg-config) #3513
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
base: main
Are you sure you want to change the base?
Changes from 21 commits
b4d3948
87aad6c
ff6aa6a
544bfa7
38f02dc
b0a5201
98edf09
3584237
dcb0115
2caa303
d8a96f8
8283906
da99a6e
17d08a9
07cbc08
c056eb5
ce9f29c
dbb3c4f
bbdc7e6
730e0f5
2e27193
debee1b
bb74239
90618ae
0c84638
a169446
5b48c26
d023fa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| #=============================================================================== | ||
| # Copyright contributors to the oneDAL project | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| #=============================================================================== | ||
|
|
||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| exports_files([ | ||
| "vars_lnx.sh", | ||
| "vars_mac.sh", | ||
| "vars_win.bat", | ||
| ]) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #=============================================================================== | ||
| # Copyright contributors to the oneDAL project | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| #=============================================================================== | ||
|
|
||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| exports_files([ | ||
| "pkg-config.cpp", | ||
| ]) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,7 +159,8 @@ def dal_test(name, hdrs=[], srcs=[], dal_deps=[], dal_test_deps=[], | |
| extra_deps=[], host_hdrs=[], host_srcs=[], host_deps=[], | ||
| dpc_hdrs=[], dpc_srcs=[], dpc_deps=[], compile_as=[ "c++", "dpc++" ], | ||
| framework="catch2", data=[], tags=[], private=False, | ||
| mpi=False, ccl=False, mpi_ranks=0, args=[], **kwargs): | ||
| mpi=False, ccl=False, mpi_ranks=0, args=[], | ||
| use_onedal_release_libs=True, **kwargs): | ||
| # TODO: Check `compile_as` parameter | ||
| # TODO: Refactor this rule once decision on the tests structure is made | ||
| if not framework in ["catch2", "none"]: | ||
|
|
@@ -179,7 +180,7 @@ def dal_test(name, hdrs=[], srcs=[], dal_deps=[], dal_test_deps=[], | |
| compile_as = compile_as, | ||
| dal_deps = ( | ||
| dal_test_deps + | ||
| _test_link_mode_deps(dal_deps) | ||
| _test_link_mode_deps(dal_deps, use_onedal_release_libs) | ||
| ) + ([ | ||
| "@onedal//cpp/oneapi/dal/test/engine:common", | ||
| "@onedal//cpp/oneapi/dal/test/engine:catch2_main", | ||
|
|
@@ -287,24 +288,26 @@ def dal_collect_parameters(name, root, modules=[], target="parameters", dal_deps | |
| **kwargs, | ||
| ) | ||
|
|
||
| def dal_example(name, dal_deps=[], **kwargs): | ||
| def dal_example(name, dal_deps=[], use_onedal_release_libs=True, is_daal=False, **kwargs): | ||
| dal_test( | ||
| name = name, | ||
| dal_deps = [ | ||
| "@onedal//cpp/oneapi/dal:core", | ||
| "@onedal//cpp/oneapi/dal/io", | ||
| ] + dal_deps, | ||
| ] + dal_deps if not is_daal else dal_deps, | ||
| use_onedal_release_libs = use_onedal_release_libs, | ||
|
||
| framework = "none", | ||
| **kwargs, | ||
| ) | ||
|
|
||
| def dal_example_suite(name, srcs, **kwargs): | ||
| def dal_example_suite(name, srcs, is_daal=False, **kwargs): | ||
| suite_deps = [] | ||
| for src in srcs: | ||
| _, alg_name, src_file = src.rsplit('/', 2) | ||
| example_name, _ = paths.split_extension(src_file) | ||
| dal_example( | ||
| name = example_name, | ||
| is_daal = is_daal, | ||
| srcs = [ src ], | ||
| **kwargs, | ||
| ) | ||
|
|
@@ -314,31 +317,33 @@ def dal_example_suite(name, srcs, **kwargs): | |
| tests = suite_deps, | ||
| ) | ||
|
|
||
| def dal_algo_example_suite(algos, dal_deps=[], **kwargs): | ||
| def dal_algo_example_suite(algos, dal_deps=[], is_daal=False, **kwargs): | ||
| for algo in algos: | ||
| dal_example_suite( | ||
| name = algo, | ||
| is_daal = is_daal, | ||
| srcs = native.glob(["source/{}/*.cpp".format(algo)]), | ||
| dal_deps = dal_deps + [ | ||
| "@onedal//cpp/oneapi/dal/algo:{}".format(algo), | ||
| ], | ||
| **kwargs, | ||
| ) | ||
|
|
||
| def _test_link_mode_deps(dal_deps): | ||
| def _test_link_mode_deps(dal_deps, use_onedal_release_libs=True): | ||
| return _select({ | ||
| "@config//:test_link_mode_dev": dal_deps, | ||
| "@config//:test_link_mode_release_static": [ | ||
| "@onedal_release//:onedal_static", | ||
| ], | ||
| ] if use_onedal_release_libs else [], | ||
| "@config//:test_link_mode_release_dynamic": [ | ||
| "@onedal_release//:onedal_dynamic", | ||
| ], | ||
| ] if use_onedal_release_libs else [], | ||
| }) | ||
|
|
||
| def _test_deps_on_daal(): | ||
| return _select({ | ||
| "@config//:test_link_mode_dev": [ | ||
| "@onedal//cpp/daal:core_static", | ||
| "@onedal//cpp/daal:threading_static", | ||
| ], | ||
| "@config//:test_link_mode_release_static": [ | ||
|
|
@@ -600,3 +605,29 @@ def _expand_select(deps): | |
| else: | ||
| expanded += [dep] | ||
| return expanded | ||
|
|
||
| def daal_example_suite(name, srcs, **kwargs): | ||
| dal_example_suite( | ||
| name = name, | ||
| srcs = srcs, | ||
| use_onedal_release_libs = False, | ||
| is_daal = True, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| def daal_algo_example_suite(algos, dal_deps=[], **kwargs): | ||
| """Build DAAL example suites using classic DAAL kernel deps. | ||
|
|
||
| Unlike dal_algo_example_suite(), this function does NOT add oneAPI | ||
| (@onedal//cpp/oneapi/dal/algo/...) targets — DAAL examples use daal.h | ||
| and depend on DAAL kernels (@onedal//cpp/daal/src/algorithms/<algo>:kernel). | ||
| """ | ||
| for algo in algos: | ||
| daal_example_suite( | ||
| name = algo, | ||
| srcs = native.glob(["source/{}/*.cpp".format(algo)]), | ||
| dal_deps = dal_deps + [ | ||
| "@onedal//cpp/daal/src/algorithms/{}:kernel".format(algo), | ||
| ], | ||
| **kwargs, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ cc_library( | |||||||||||||||||
| hdrs = glob([ | ||||||||||||||||||
| "include/**/*.h", | ||||||||||||||||||
| "include/oneapi/**/*.hpp", | ||||||||||||||||||
| ], allow_empty=True), | ||||||||||||||||||
| ]), | ||||||||||||||||||
| includes = [ "include" ], | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -43,15 +43,26 @@ cc_library( | |||||||||||||||||
| ], | ||||||||||||||||||
| deps = [ | ||||||||||||||||||
| ":headers", | ||||||||||||||||||
|
||||||||||||||||||
| ":headers", | |
| ":headers", | |
| "@mkl//:mkl_static", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the later comment on this topic: MKL is statically embedded in the prebuilt oneDAL archives via ar -M. Adding @mkl//:mkl_static would cause duplicate symbol linking.
Copilot
AI
Mar 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onedal_static wraps prebuilt static archives (libonedal.a, libonedal_parameters.a) but currently has no dependency on MKL/TBB libraries. Consumers that link only @onedal_release//:onedal_static (as dal_test does in --test_link_mode=release_static) may get unresolved symbols unless the required runtime libs are pulled in elsewhere. Consider adding the required deps (e.g., MKL static + TBB) here so the target is self-contained for linking.
| ":headers", | |
| ":headers", | |
| "@tbb//:tbb_binary", | |
| "@tbb//:tbbmalloc_binary", | |
| "@mkl//:mkl_static", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declining this suggestion. In release mode, MKL is statically embedded into the prebuilt oneDAL archives at build time via ar -M (the same mechanism Make uses). The onedal_static target wraps these prebuilt archives where MKL symbols are already fully included. Adding @mkl//:mkl_static here would cause MKL to be linked twice, producing duplicate symbol errors.
Copilot
AI
Mar 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using glob(["lib/intel64/libonedal_core.so*"]) will match the unversioned .so symlink plus the versioned .so.<major> symlink and the real .so.<major>.<minor> file produced by the release rule. That means consumers of :core_dynamic may end up linking the same library multiple times. Consider selecting a single file for linking (e.g., the real .so.<major>.<minor>), and expose the other symlinks via a separate filegroup/data if they’re needed at runtime.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core_dynamic/thread_dynamic use glob() for a single expected versioned .so file. This can silently succeed with an empty match (depending on Bazel settings) and later fail in less obvious ways; it’s safer to reference the exact expected file path (with the substituted version placeholders) so missing artifacts fail fast.
Copilot
AI
Mar 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glob(["lib/intel64/libonedal.so*", ...]) will match the unversioned symlink, the major-version symlink, and the real *.so.X.Y file once the release rule starts creating all three. Passing multiple matching .so files to the same cc_library can lead to duplicate/ambiguous link inputs. Prefer selecting a single shared object (e.g., only the real *.so.%{version_binary_major}.%{version_binary_minor} like core_dynamic/thread_dynamic, or only the unversioned .so symlink) for each cc_library target.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onedal_dynamic now globs libonedal.so* / libonedal_parameters.so*, which will include the real file plus the .so.<major> and .so symlinks. Passing multiple aliases of the same shared library as inputs is unnecessary and can lead to duplicate linker inputs; prefer listing just the intended link name (typically the unversioned .so) or a single, specific versioned file.
Copilot
AI
Mar 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libonedal_parameters_dpc.so* will now match multiple files after this PR adds versioned .so plus .so.<major> and .so symlinks. That can feed duplicate linker inputs into onedal_dynamic_dpc. Prefer a single deterministic input (e.g., the fully versioned .so.<major>.<minor> like libonedal_dpc, or only the unversioned .so symlink) to avoid accidental multi-match behavior.
| "lib/intel64/libonedal_parameters_dpc.so*", | |
| "lib/intel64/libonedal_parameters_dpc.so.%{version_binary_major}.%{version_binary_minor}", |
Copilot
AI
Mar 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libonedal_parameters_dpc.so* will match both the real versioned file and the .so.<major> / .so symlinks introduced by the release packaging, which can add duplicate shared-library inputs and (if SONAME is missing) inconsistent DT_NEEDED names. Consider narrowing this (and similarly the libonedal.so* / libonedal_parameters.so* globs in onedal_dynamic) to only the real .so.%{version_binary_major}.%{version_binary_minor} file to avoid pulling symlinks into link inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is on an outdated diff. The glob was narrowed to the exact versioned file (.so.X.Y) in a subsequent commit to avoid pulling symlinks into link inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//:releasenow always depends onrelease_vars_shandrelease_pkgconfig. If//:releaseis built on unsupported platforms (e.g., Windows), these targets will currently generate Unix shell artifacts and/or requiregcc. Consider guarding these with platform constraints or adding a Windows-specific implementation before making them unconditional.