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.

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

Automatically use embed-manifest-via=linker for clang-win #159

Closed
Flamefire opened this issue Jun 8, 2022 · 9 comments · Fixed by #385
Closed

Automatically use embed-manifest-via=linker for clang-win #159

Flamefire opened this issue Jun 8, 2022 · 9 comments · Fixed by #385
Labels
enhancement New feature or request

Comments

@Flamefire
Copy link
Contributor

Environment and version details

Github Actions "Windows 2022" image.

Describe your use case

Building Boost with toolset=clang-win results in a failure:

'"C:\Program Files (x86)\Windows Kits\10\bin\x64\mt.exe"' is not recognized as an internal or external command,
operable program or batch file.

Adding embed-manifest-via=linker solves this.

Describe the solution you'd like

Seemingly the b2 toolset uses a path to the non-existing file. For usability it would be good if B2 automatically uses embed-manifest-via=linker at least when mt.exe isn't found.

@pdimov
Copy link
Contributor

pdimov commented Jun 8, 2022

I can't think of any downside to making embed-manifest-via=linker the default unconditionally across the board, so that the manifest tool is no longer needed.

@Kojoley
Copy link
Contributor

Kojoley commented Jun 8, 2022

I can't think of any downside to making embed-manifest-via=linker the default unconditionally across the board, so that the manifest tool is no longer needed.

It requires VC11+

@pdimov
Copy link
Contributor

pdimov commented Jun 8, 2022

@Kojoley
Copy link
Contributor

Kojoley commented Jun 8, 2022

Interesting. I use it unconditionally: boostorg/container_hash@87c9eef/.appveyor.yml#L72

and the msvc-9.0,msvc-10.0 jobs pass: ci.appveyor.com/project/pdimov/container-hash/build/job/tbqpop4ap9qveye6

I believe your input is saliently overridden by B2 for VC<11.

@Flamefire
Copy link
Contributor Author

I believe your input is saliently overridden by B2 for VC<11.

Why do you think that and how is it overridden? I mean there is compile-c-c++ bin.v2\libs\container_hash\test\hash_number_test2.test\msvc-9.0\debug\address-model-32\threading-multi\hash_number_test2.obj

Anyway the current solution is a clear Bug: The manifest-tool doesn't exist, the linker is capable of embedding the manifest but B2 still tries to run a non-existing tool.

So 2 solutions:

  1. B2 tries the linker first to see if it is capable of embedding the manifest (likely succeeds in almost all cases nowadays) and only if not searches for the mt.exe and errors out if it doesn't exist
  2. B2 searches for mt.exe and if that is not found enables embed-manifest-via=linker

I'd prefer the first solution as it is likely the most efficient one and provides a clean error in case of failure. The second is likely easier to implement.

@pdimov
Copy link
Contributor

pdimov commented Jun 9, 2022

I believe Nikita means

b2/src/tools/msvc.jam

Lines 602 to 617 in 0c49944

if [ version.version-less [ SPLIT_BY_CHARACTERS [ MATCH "^([0123456789.]+)" : $(version) ] : . ] : 11 ]
{
# Make sure that manifest will be generated even if there is no
# dependencies to put there.
toolset.flags $(toolset).link LINKFLAGS $(conditions) : /MANIFEST ;
}
else
{
toolset.flags $(toolset).link LINKFLAGS $(conditions)/<embed-manifest-via>mt : /MANIFEST ;
toolset.flags $(toolset).link LINKFLAGS $(conditions)/<embed-manifest-via>linker/<embed-manifest>off : /MANIFEST ;
toolset.flags $(toolset).link LINKFLAGS $(conditions)/<embed-manifest-via>linker/<embed-manifest>on : "/MANIFEST:EMBED" ;
local conditionx = [ feature.split $(conditions) ] ;
toolset.add-defaults $(conditionx:J=,)\:<embed-manifest-via>linker ;
}
}
. Not sure what the intended meaning of "saliently" here was. :-)

The above does seem to set linker as the default though. Is this a recent change? Seems not:

b613e6d

So we only need to make this default for clang-win, as it's already default for msvc.

@Flamefire
Copy link
Contributor Author

Flamefire commented Jun 9, 2022

So we only need to make this default for clang-win, as it's already default for msvc.

Hm but there seemingly was an issue:

Unfortunately, it cannot be enabled on clang-cl with MSVC linker at the moment
because it because of some path issues:

However given that we need it now, I'd say it no longer is an issue.

@pdimov
Copy link
Contributor

pdimov commented May 1, 2024

Recent fixes to clang-win.jam may have made this unnecessary, but I think embed-manifest-via=linker should still be the default.

@Kojoley
Copy link
Contributor

Kojoley commented May 4, 2024

embed-manifest-via=linker is broken on clang-win, it disables manifest embedding completely (disables mt invocation, but doesn't add /manifest:embed to link flags). Fixing that will expose all uses of embed-manifest-via=linker to a clang bug: it doesn't put WinSDK in PATH before invoking link.exe /manifest:embed, so you get LINK : fatal error LNK1158: cannot run 'rc.exe', which can be workaround but then there will still be no point in defaulting to embed-manifest-via=linker.

lld-link from recent (17?) release doesn't use WinSDK anymore, so clang-cl.exe /link /manifest:embed no longer fails with lld-link: error: unable to find mt.exe in PATH: no such file or directory. Switching to lld-link on clang-win is also needed to use LTO, so it might be a good move now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants