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

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

Open
pdimov opened this issue May 1, 2024 · 15 comments
Open

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

pdimov opened this issue May 1, 2024 · 15 comments
Labels
enhancement New feature or request

Comments

@pdimov
Copy link
Contributor

pdimov commented May 1, 2024

With boostorg/boost#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 pdimov added the enhancement New feature or request label May 1, 2024
@pdimov
Copy link
Contributor Author

pdimov commented May 1, 2024

I can't think of any.

Well... #325.

@Kojoley
Copy link
Contributor

Kojoley commented May 1, 2024

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 boostorg/context#228 and it got merged but then reverted because of b2 bug, fix for which didn't make to Boost repository quickly enough.

@grisumbras
Copy link
Contributor

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

As described in #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
Copy link
Contributor

Kojoley commented May 1, 2024

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 #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
Copy link
Contributor

grisumbras commented May 1, 2024

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
Copy link
Contributor

Kojoley commented May 1, 2024

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
Copy link
Contributor Author

pdimov commented May 1, 2024

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
Copy link
Contributor

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

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
Copy link
Contributor

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

@pdimov
Copy link
Contributor Author

pdimov commented May 1, 2024

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
Copy link
Contributor Author

pdimov commented May 1, 2024

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
Copy link
Contributor Author

pdimov commented May 1, 2024

We're looking at implementing the x86_64/ suggestion above on the Boost side, via boostorg/boost#898.

@grafikrobot
Copy link
Member

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
Copy link
Contributor Author

pdimov commented May 1, 2024

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
Copy link
Contributor

Kojoley commented May 7, 2024

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 #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.

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
None yet
Development

No branches or pull requests

4 participants