bootstrap: use internment instead of hand-rolled interning

[GrigorenkoPV]

Part of the #109859.

[rustbot]

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[bors]

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[GrigorenkoPV]

Unfortunately, this pulls in hashbrown, so probably not worth it

[bors]

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #130951) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@GrigorenkoPV what's the status on this? maybe you can resolve the conflicts and we can push this forward

[GrigorenkoPV]

@rustbot ready

[rust-log-analyzer]

The job mingw-check-tidy failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/
# NOTE: intentionally uses python2 for x.py so we can test it still works.
# validate-toolstate only runs in our CI, so it's ok for it to only support python3.
ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test \
--stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
# pip-compile --allow-unsafe --generate-hashes reuse-requirements.in
---
#13 3.232 Building wheels for collected packages: reuse
#13 3.233 Building wheel for reuse (pyproject.toml): started
#13 3.478 Building wheel for reuse (pyproject.toml): finished with status 'done'
#13 3.479 Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132720 sha256=026f3bb0f1aa8090b861fd0a0939cb1a782396d84c8aab7875096557d637a0f6
#13 3.479 Stored in directory: /tmp/pip-ephem-wheel-cache-kfv0f51x/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#13 3.482 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#13 3.867 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#13 3.868 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#13 4.415 Collecting virtualenv
#13 4.415 Collecting virtualenv
#13 4.473 Downloading virtualenv-20.27.1-py3-none-any.whl (3.1 MB)
#13 4.697 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.1/3.1 MB 14.1 MB/s eta 0:00:00
#13 4.741 Collecting distlib<1,>=0.3.7
#13 4.752 Downloading distlib-0.3.9-py2.py3-none-any.whl (468 kB)
#13 4.771 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 469.0/469.0 KB 26.4 MB/s eta 0:00:00
#13 4.813 Collecting filelock<4,>=3.12.2
#13 4.823 Downloading filelock-3.16.1-py3-none-any.whl (16 kB)
#13 4.864 Collecting platformdirs<5,>=3.9.1
#13 4.874 Downloading platformdirs-4.3.6-py3-none-any.whl (18 kB)
#13 4.954 Installing collected packages: distlib, platformdirs, filelock, virtualenv
#13 5.130 Successfully installed distlib-0.3.9 filelock-3.16.1 platformdirs-4.3.6 virtualenv-20.27.1
#13 DONE 5.2s
#14 [7/8] COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
#14 DONE 0.0s
---
DirectMap4k: 331712 kB
DirectMap2M: 8056832 kB
DirectMap1G: 10485760 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint,cpp:fmt
Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
WARNING: `rust.download-rustc` is enabled. The `rust.channel` option will be overridden by the CI rustc's channel.
downloading https://static.rust-lang.org/dist/2024-10-16/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.xz
---
##[endgroup]
fmt check
fmt: checked 5659 files
tidy check
tidy error: invalid license `Zlib` in `registry+https://github.com/rust-lang/crates.io-index#foldhash@0.1.3`
removing old virtual environment
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10'
Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (24.3.1)
All checks passed!
checking C++ file formatting
some tidy checks failed
some tidy checks failed
Command has failed. Rerun with -v to see more details.
local time: Sun Nov 10 10:36:10 UTC 2024
network time: Sun, 10 Nov 2024 10:36:10 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[jieyouxu]

tidy error: invalid license Zlib in registry+https://github.com/rust-lang/crates.io-index#foldhash@0.1.3

I'll ask T-compiler leads regarding the license, AFAICT foldhash is Zlib-only, not co-licensed with Apache/MIT.

[GrigorenkoPV]

Well, it comes as a dependency from the hashbrown crate. And no, it does not seem to be dual licensed.

Anyways, I'm not sure, what do I have to do now? Since it's S-waiting-on-author

[jieyouxu]

Sorry, I meant to change to S-waiting-for-compiler-leads (S-blocked) :3 because:

• If it's allowed, then we can just update the tidy allowed licenses.
• If it's not allowed, we need to fix hashbrown.

[Kobzol]

While I like the code simplification, as usually, adding a dependency to bootstrap is a very hard sell. This increases the number of dependencies from 86 to 91, for relatively little gain. This code was written in ~2017, and from that time it required only very little maintenance (apart from some Clippy lint fixes and some small refactoring), and it seems to be working just fine. Yes, we found a case of unsoundness in it once, but now that the two unsafe lines of code in it were heavily reviewed, I trust that it should be OK.

CC also @onur-ozkan

[jieyouxu]

While I like the code simplification, as usually, adding a dependency to bootstrap is a very hard sell. This increases the number of dependencies from 86 to 91, for relatively little gain.

For more context on why, bootstrap needs to keep minimal deps as it needs to build fast.

[onur-ozkan]

While I like the code simplification, as usually, adding a dependency to bootstrap is a very hard sell. This increases the number of dependencies from 86 to 91, for relatively little gain. This code was written in ~2017, and from that time it required only very little maintenance (apart from some Clippy lint fixes and some small refactoring), and it seems to be working just fine. Yes, we found a case of unsoundness in it once, but now that the two unsafe lines of code in it were heavily reviewed, I trust that it should be OK.

CC also @onur-ozkan

Completely agree.

[Kobzol]

The original issue has mentioned the lasso library, which seems to only depend on hashbrown. But we don't even depend on that :( So with lasso, it would actually be 94.

We should probably make a t-bootstrap decision on what to do about #109859, to avoid sending mixed signals that we want to remove all unsafe at all costs, which is probably not true.

[Mark-Simulacrum]

Yeah, I'm guessing we should just close the issue - I also don't think removing (moving!) all unsafe code to dependencies is a particularly worthwhile goal in and of itself. In this particular case as others have said the code works and is pretty cheap to maintain. I don't think a dependency benefits us enough to move to it.

[GrigorenkoPV]

I also do not think moving unsafe over into dependencies would be a good idea in all cases. I just mostly wanted to get it over with #109859 by either fixing or getting an explicit "wontfix" for every point brought up there. Just to make sure that all the unaddressed places were consciously not touched and not just simply overlooked. So, now that #109859 is closed,

[Kobzol]

Sorry for the previous confusion. Thank you for working on this, and making the other changes that were already merged!

[jieyouxu]

Yeah, we should've updated that issue earlier. Sorry for the confusion and thanks for the PR!