bfgroup / b2

B2 makes it easy to build C++ projects, everywhere.
https://www.bfgroup.xyz/b2/
Boost Software License 1.0
75 stars 228 forks source link

Should --abbreviate-paths be made default under Windows? #378

Open pdimov opened 4 months ago

pdimov commented 4 months ago

With https://github.com/boostorg/boost/pull/894 being merged, paths now acquire the additional address-model-64/architecture-x86/, which means increased probability of hitting the dreaded Windows path length limit.

Is there a downside of making --abbreviate-paths on by default on Windows? I can't think of any.

pdimov commented 4 months ago

I can't think of any.

Well... https://github.com/bfgroup/b2/issues/325.

Kojoley commented 4 months ago

On the one side it makes sense, msvc compiler and linker doesn't support long path even with windows opt-in long path support feature. But on the other, it's more like a Boost issue. The only Boost library that needs explicit address-model and architecture values is Context. I've summited PR that removed need in them https://github.com/boostorg/context/pull/228 and it got merged but then reverted because of b2 bug, fix for which didn't make to Boost repository quickly enough.

grisumbras commented 4 months ago

This is not really a Boost issue. Let's unwrap the issue in full.

As described in bfgroup/b2#368, ac module doesn't find OpenSSL libraries. The module finds a library by trying to link to it. In this case linking fails, because it doesn't use the correct flag (-m32).

In order for b2 to use that flag both address-model=32 and an appropriate architecture=A properties have to be present in the requested property set. Boost has been overcoming this inconvenience by using special deduced-X composed features that 1) add X property to the property set if X is missing (and deduction suceeded), and 2) hide the deduced value from target paths. This works for regular targets.

Unfortunately, it doesn't work for ac targets. This because the targets aren't built when the full dependency graph is constructed and all property sets are fully simplified to their constituting features. They are built as part of constructing the property sets and build graphs. The success of their builds affects the graphs and usage requirements of other targets. So, they are built as part of evaluation of conditional properties.

Conditional properties are evaluated several times, because they can have order of evaluation dependency: property P1 adds property P2 to the property set, property P3 adds P4 to the property set, but only if P2 is in it. So, evaluating P1 then P3 gives P1 P2 P3. But evaluating P3 then P1 gives P1 P2 P3 P4. Due to this effect, ac targets often have to be built several times for different property sets. In the concrete example of Boost.MySQL, the user requests address-model=32, the module ac constructs targets to check if /openssl//ssl can be linked to with address-model=32; then, during the second round of evaluation of conditionals, it does the same only with property set address-model=32 deduced-architecture=x86. The first should fail to build, the second should succeed.

But the second will not in fact be built again. This is because deduced-architecture is a hidden feature, and does not affect the target path. So, /openssl//ssl/<address-model>32 and /openssl//ssl/<address-model>32,<deduced-architecture>x86 lead to building the same JAM targets , something like <pbin.v2/gcc-12/address-model-32/debug/threading-multi>ssl. After the first attempt, the build system has determined that that target has failed to build and will not attempt to do it again in the same run.

This is why boostorg/boost#894 removes those hidden features and relies on regular features instead. The issue fundamentally isn't tied to Boost at all.

What are the potential ways to fix the issue, while keeping hidden features? We could change gcc.jam to add the flag -m32 when there's no architecure feature in the property set. Effectively, the build system would presume a compatible host. Another way to fix this is making low-level b2 calls to UPDATE_NOW to ignore whether the updated target has been built or not. Both sound very hacky to me. Hence, the removal of hidden features.

BTW, in theory, this isn't just a problem with ac targets. Let's say, somehow a regular target is built with 2 property sets: one has <address-model>32 and another has <address-model>32 <deduced-architecture>x86. We are back to the same problem: the first one to build will determine the success of both. I can't imagine why someone would deliberately do this, so this probably can never happen in correctly written build scripts, but can mainfest in badly written ones.

Kojoley commented 4 months ago

In order for b2 to use that flag both address-model=32 and an appropriate architecture=A properties have to be present in the requested property set.

I consider that a bug in b2.

But the second will not in fact be built again. This is because deduced-architecture is a hidden feature, and does not affect the target path. So, /openssl//ssl/<address-model>32 and /openssl//ssl/<address-model>32,<deduced-architecture>x86 lead to building the same JAM targets , something like <pbin.v2/gcc-12/address-model-32/debug/threading-multi>ssl. After the first attempt, the build system has determined that that target has failed to build and will not attempt to do it again in the same run.

Hmmm, maybe https://github.com/bfgroup/b2/pull/303 was a wrong idea, I don't know.

What are the potential ways to fix the issue, while keeping hidden features? We could change gcc.jam to add the flag -m32 when there's no architecure feature in the property set.

I have a patch in the stash that does that. I think it makes no sense that address-model is ignored unless architecture is set too. From the change history it was not initially, but it was causing issues for other architectures.

grisumbras commented 4 months ago

Philosophically it is not correct to add -m32 unless the target architecture is known to be appropriate. Whether it is practically better is debateable. Consider someone building for their architecture that is not well supported by b2. They put using gcc : : : <compileflags>-m32am : <address-model>32 ; into their configs. They run the build with b2 address-model=32, only to see gcc -m32 -m32am in the logs. Now what do they do?

Kojoley commented 4 months ago

Philosophically it is not correct to add -m32 unless the target architecture is known to be appropriate. Whether it is practically better is debateable. Consider someone building for their architecture that is not well supported by b2. They put using gcc : : : <compileflags>-m32am : <address-model>32 ; into their configs. They run the build with b2 address-model=32, only to see gcc -m32 -m32am in the logs. Now what do they do?

That's a straw man. I didn't propose to add -m32 for every architecture, GCC supports it only for a few arches. But it also doesn't care if you put multiple -m flags (https://godbolt.org/z/7dYz16ExW), the last will win, so your example is fine, I guess.

I honestly consider address-model a mistake. The only legitimate use-case for it would be x32 abi which doesn't fit into current scheme either, but the ship has sailed.

pdimov commented 4 months ago

I honestly consider address-model a mistake.

What do you mean by that? That 32 bit should be a separate architecture?

How would that solve our problem?

grisumbras commented 4 months ago

1.

That's a straw man. I didn't propose to add -m32 for every architecture,

Well, then what does this mean?

We could change gcc.jam to add the flag -m32 when there's no architecure feature in the property set. I have a patch in the stash that does that.

But it also doesn't care if you put multiple -m flags

2. I just tried

gcc -c 1.cpp -maix32 -m32 -o 1.o

And got

gcc: error: unrecognized command-line option ‘-maix32’; did you mean ‘-mavx2’?

So, adding wrong flags only works if GCC has been configured to recognise them. Which is not necessarily the case. So, again, maybe practically things will work, maybe they won't. And fixing it on the user side would probably require patching gcc.jam.

Although, doing something like this should probably work:

feature.extend architecture : myarch ;
toolset.flags gcc.compile OPTIONS <target-os>linux/<architecture>myarch/<address-model>32 : -m32am ;
toolset.flags gcc.link OPTIONS <target-os>linux/<architecture>myarch/<address-model>32 : -m32am ;

But it suddenly requires much more understanding of b2 from the user.

grisumbras commented 4 months ago

379 Oh, you meant, you wanted to add architecture deduction to toolset modules. I see.

pdimov commented 4 months ago

Another option is to special-case architecture and address-model and instead of adding address-model-64/architecture-x86/ to the path, just add x86-64/ to it.

pdimov commented 4 months ago

Yet another option is to have a max property set path length threshold in b2.exe and switch to a hash automatically once that is hit.

Unfortunately we'll have to set some arbitrary constant there because the combined path depends on the current directory. But it will be better than nothing.

pdimov commented 4 months ago

We're looking at implementing the x86_64/ suggestion above on the Boost side, via https://github.com/boostorg/boost/pull/898.

grafikrobot commented 4 months ago

IMO.. We should make the hash mode the default on Windows if we truly want to resolve this. Anything else just postpones the problem.

pdimov commented 4 months ago

Hiding the paths isn't ideal; it's useful to see what's being built. At minimum we should keep the paths that don't exceed the hash length (32), but the budget can also be set a bit higher (e.g. 40).

Kojoley commented 3 months ago

I have an idea that can alleviate the issue and it involves a change that I've wanted to make for a while: feature minimization should consider toolset defaults and requirements (toolset.add-defaults/toolset.add-requirements) as default property values.

That will allow msvc toolset to do toolset.add-defaults <toolset>msvc:<architecture>$(host-arch) <toolset>msvc:<address-model>$(host-addrmdl) which would make property-set.as-path to not produce the address-model-64/architecture-x86/ part even when they were explicitly set.

But such minimization might open a can of worms: cache could become invalid suddenly, in a way like free features currently are considered like incidental for a target uniqueness/path (which is a build cache key, and I think is a serious b2 bug that I might have already exposed users to by https://github.com/bfgroup/b2/pull/303). While adding such toolset defaults might make users assume that <architecture> and <address-model> are always defined and rely on them in their scripts.