fix: Add runtime_library_search_directories for QNX dynamic linking

[jgetas]

QNX toolchain requires -Wl,-rpath instead of -rpath for runtime library paths. This enables cc_shared_library and dynamic linking support.

Extend examples for dynamic linking case.

[nradakovic]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

[nradakovic]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

Also, I don't see set on toolchains_qnx. Is there a reason why this was not enabled there?

[jgetas]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

Also, I don't see set on toolchains_qnx. Is there a reason why this was not enabled there?

Yes it's only with the qcc/q++ compiler. Works on gcc/linux without changes. And yes, we identified it first (last week) on toolchains_qnx. Because it was already being phased out we checked with bazel_cpp_toolchains. Luckily fixing it there was possible with limited impact.

What kind of guard do you mean? The change is in template/qnx so it should be limited to QNX only. Please make a proposal - I may not get what you mean...

[jgetas]

FYI: The error sighting in building the examples with the math_lib_dyn_test without the runtime_library_search_directories_feature is:

$ bazel build --config x86_64-qnx  --action_env=QNXLM_LICENSE_FILE=[...] //...
INFO: Analyzed 6 targets (73 packages loaded, 21418 targets configured).
ERROR: [...]/bazel_cpp_toolchains/examples/BUILD:47:8: Linking math_lib_dyn_test failed: (Exit 1): q++ failed: error executing CppLink command (from target //:math_lib_dyn_test) external/score_bazel_cpp_toolchains++gcc+etas_sdp803_pkg/host/linux/x86_64/usr/bin/q++ @bazel-out/k8-fastbuild/bin/math_lib_dyn_test-0.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
cc: unknown option: -rpath

INFO: Elapsed time: 9.682s, Critical Path: 6.84s
INFO: 25 processes: 15 internal, 10 linux-sandbox.
ERROR: Build did NOT complete successfully

[jgetas]

...and one more comment for explanation:

My guess for the main root cause is that bazel cc_rules don't really support qcc well. When trying to trace the toolchains_qnx case we googled and stumbled over https://github.com/bazelbuild/rules_cc/blob/e6d30a5681b7d1916b9a6e36cb3d8798d7a9d4c1/cc/private/toolchain/unix_cc_toolchain_config.bzl#L807 . I am not really sure if this rule is applied in our case, but we found the -Xlinker -rpath ... in the options and tried to use that with qcc which failed.

In the rules_cc link above the -Xlinker -rpath ... is used for Linux which works there. But it's also used in the else case. And that apparently does not work for qcc.

So the main root cause could be that bazel cc_rules don't really support qcc and this PR is a workaround for that.

[nradakovic]

...and one more comment for explanation:

My guess for the main root cause is that bazel cc_rules don't really support qcc well. When trying to trace the toolchains_qnx case we googled and stumbled over https://github.com/bazelbuild/rules_cc/blob/e6d30a5681b7d1916b9a6e36cb3d8798d7a9d4c1/cc/private/toolchain/unix_cc_toolchain_config.bzl#L807 . I am not really sure if this rule is applied in our case, but we found the -Xlinker -rpath ... in the options and tried to use that with qcc which failed.

In the rules_cc link above the -Xlinker -rpath ... is used for Linux which works there. But it's also used in the else case. And that apparently does not work for qcc.

So the main root cause could be that bazel cc_rules don't really support qcc and this PR is a workaround for that.

It's actually a bit deeper than that. Most configurations (this one as well) relay on legacy features. These features are used as autocomplete compiler and linker configuration and if you do not override them, Bazel assumes you want to use default implementation. For that he uses Unix like build and Windows (I think) like build to configure "missing" features. In case of QNX they assume search paths are the same as on Linux.

Solution is of course to disable this, and go full customized configuration, but I have no time to implement that. I have done it for internal toolchains this approach, it's for sure better and safer, but for S-CORE I don't have time.

[nradakovic]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

Also, I don't see set on toolchains_qnx. Is there a reason why this was not enabled there?

Yes it's only with the qcc/q++ compiler. Works on gcc/linux without changes. And yes, we identified it first (last week) on toolchains_qnx. Because it was already being phased out we checked with bazel_cpp_toolchains. Luckily fixing it there was possible with limited impact.

What kind of guard do you mean? The change is in template/qnx so it should be limited to QNX only. Please make a proposal - I may not get what you mean...

We have idea that same features exists on both configuration so that in the future we can merge templates. We tried already now but too many delta were in place so we went separate template. But with this said, I don't want to make different linker features between them. Can we set this in default linker options?

[jgetas]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

Also, I don't see set on toolchains_qnx. Is there a reason why this was not enabled there?

Yes it's only with the qcc/q++ compiler. Works on gcc/linux without changes. And yes, we identified it first (last week) on toolchains_qnx. Because it was already being phased out we checked with bazel_cpp_toolchains. Luckily fixing it there was possible with limited impact.
What kind of guard do you mean? The change is in template/qnx so it should be limited to QNX only. Please make a proposal - I may not get what you mean...

We have idea that same features exists on both configuration so that in the future we can merge templates. We tried already now but too many delta were in place so we went separate template. But with this said, I don't want to make different linker features between them. Can we set this in default linker options?

Sure - if you have a better way to implement this, that's completely fine with me. We can discard this PR in that case. That was just how we thought a broken feature we require could be fixed.

[nradakovic]

Is this only for QNX? If that's the case, we need to set some guard, or to move this as part of template enablement.

Also, I don't see set on toolchains_qnx. Is there a reason why this was not enabled there?

Yes it's only with the qcc/q++ compiler. Works on gcc/linux without changes. And yes, we identified it first (last week) on toolchains_qnx. Because it was already being phased out we checked with bazel_cpp_toolchains. Luckily fixing it there was possible with limited impact.
What kind of guard do you mean? The change is in template/qnx so it should be limited to QNX only. Please make a proposal - I may not get what you mean...

We have idea that same features exists on both configuration so that in the future we can merge templates. We tried already now but too many delta were in place so we went separate template. But with this said, I don't want to make different linker features between them. Can we set this in default linker options?

Sure - if you have a better way to implement this, that's completely fine with me. We can discard this PR in that case. That was just how we thought a broken feature we require could be fixed.

No, I don't want to take credit for your work. I just want that you extend the list of current default_linker_flags_feature instead of creating completely new feature. If this is something that is mandatory for QNX, then we put it in default link feature. That's all. Update PR and I will approve it.

[nradakovic]

Crap. With my last change I made conflict. Sorry about that.

[jgetas]

I tried with extending default_link_flags_feature instead of a separate feature, but bazel build fails with it:

    default_link_flags_feature = feature(
        name = "default_link_flags",
        enabled = True,
        flag_sets = [
            flag_set(
                actions = all_link_actions,
                flag_groups = [flag_group(flags = ["-V%{tc_version},gcc_nto%{tc_cpu}_cxx"])],
            ),
            flag_set(
                actions = all_link_actions,
                flag_groups = DEFAULT_LINK_FLAGS,
            ),
           # adding this:
            flag_set(
                actions = all_link_actions,
                flag_groups = [
                    flag_group(
                        iterate_over = "runtime_library_search_directories",
                        flag_groups = [
                            flag_group(
                                flags = ["-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}"],
                            ),
                        ],
                    ),
                ],
            ),
        ],
    )

# in examples:
bazel build --config x86_64-qnx  --action_env=QNXLM_LICENSE_FILE=[...] //...
[...]
ERROR: [...]/bazel_cpp_toolchains/examples/BUILD:47:8: Linking math_lib_dyn_test failed: (Exit 1): q++ failed: error executing CppLink command (from target //:math_lib_dyn_test) external/score_bazel_cpp_toolchains++gcc+etas_sdp803_pkg/host/linux/x86_64/usr/bin/q++ @bazel-out/k8-fastbuild/bin/math_lib_dyn_test-0.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
cc: unknown option: -rpath

File bazel-out/.../math_lib_dyn_test-0.params contains then both -rpath variants, the failing -Xlinker -rpath ... and the corrected one:

-o
bazel-out/k8-fastbuild/bin/math_lib_dyn_test
-Xlinker
-rpath
-Xlinker
$ORIGIN/_solib_x86_64/_U
-Xlinker
-rpath
-Xlinker
$ORIGIN/math_lib_dyn_test.runfiles/_main/_solib_x86_64/_U
-Lbazel-out/k8-fastbuild/bin/_solib_x86_64/_U
bazel-out/k8-fastbuild/bin/_objs/math_lib_dyn_test/math_lib_test.pic.o
-lmath_lib
-Wl,-S
-V12.2.0,gcc_ntox86_64_cxx
-Wl,-z,relro
-Wl,-z,now
-Wl,--push-state
-Wl,--as-needed
-lc++
-lm
-Wl,--pop-state
-Wl,-rpath,$EXEC_ORIGIN/_solib_x86_64/_U
-Wl,-rpath,$EXEC_ORIGIN/math_lib_dyn_test.runfiles/_main/_solib_x86_64/_U

[kgraeper]

If I have:

    runtime_library_search_directories_feature = feature(
        name = "runtime_library_search_directories",
     )

and

     default_link_flags_feature = feature(
        name = "default_link_flags",
        ...
        flag_set(
                actions = all_link_actions,
                flag_groups = [
                    flag_group(
                        iterate_over = "runtime_library_search_directories",
                        flag_groups = [
                            flag_group(
                                flags = ["-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}"],
                            ),
                        ],
                    ),
                ],
            ),

and add them to the features

   features = [
        default_link_flags_feature,
        runtime_library_search_directories_feature,
        ...
    ]

the correct linker flags are produced. So somehow (maybe via here) the flags are added and we need to disable or overwrite it.

@nradakovic What is your opinion? Overwrite or disable?

[nradakovic]

If I have:

    runtime_library_search_directories_feature = feature(
        name = "runtime_library_search_directories",
     )

and

     default_link_flags_feature = feature(
        name = "default_link_flags",
        ...
        flag_set(
                actions = all_link_actions,
                flag_groups = [
                    flag_group(
                        iterate_over = "runtime_library_search_directories",
                        flag_groups = [
                            flag_group(
                                flags = ["-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}"],
                            ),
                        ],
                    ),
                ],
            ),

and add them to the features

   features = [
        default_link_flags_feature,
        runtime_library_search_directories_feature,
        ...
    ]

the correct linker flags are produced. So somehow (maybe via here) the flags are added and we need to disable or overwrite it.

@nradakovic What is your opinion? Overwrite or disable?

Ah I was hoping that this is not one of legacy features but it is. So we have 2 options:

1. We override the legacy feature and write our own (stuff that you originaly proposed).
2. We overide the legacy feature with dummuy implementation and set our implementation inside default_link_flags (stuff that you wrote in comment).

Either way we will need to remove this once we get rid of legacy features, so I would say, leave it as is, just fix merge conflicts and once we add no_legacy_features we will move implementation to default_link_flags.

[jgetas]

@nradakovic Ok 👍 rebased to main. Ready to merge. Please approve.