This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Rename __tuple to __tuple_dir to avoid file collision
ClosedPublic

Authored by mgorny on Dec 3 2022, 11:09 PM.

Details

Summary

Rename the __tuple directory in libc++ headers to __tuple_dir
to avoid file collision when installing. Historically, __tuple has
been a file and it has been replaced by a directory
in 2d52c6bfae801b016dd3627b8c0e7c4a99405549. Replacing a regular file
with a directory (or more importantly, the other way around when
downgrading) is not universally supported. Since this is an internal
header, its actual name should not matter, so just rename it to avoid
problems.

Diff Detail

Event Timeline

mgorny created this revision.Dec 3 2022, 11:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2022, 11:09 PM
mgorny requested review of this revision.Dec 3 2022, 11:09 PM

The build fails since the generated files aren't updated. This can be done by running ninja libcxx-generate-files. Alternatively one of the libc++ maintainers can commandeer the patch to fix these issues.

libcxx/include/CMakeLists.txt
521

IIRC this used to be a file to, is that also an issue?

I've regenerated files. However, I needed to modify the generator to strip the extra 2 from public header name. That's not the prettiest solution, I admit, so I'm open to better ideas.

libcxx/include/CMakeLists.txt
521

Yeah, it was. However, since it's in 15.x already, I don't have a strong opinion wrt fixing it now. It only occurred to me now that there's a relatively simple solution to the problem.

If you think we should change that too, then I can make a patch (perhaps separate from this one?).

mgorny updated this revision to Diff 479936.Dec 4 2022, 11:10 AM

Regenerate files.

From the patch, I'm not sure I understand what's the process you are expecting us to follow for renaming a file to a directory. Are you suggesting that we rename it to __tuple2 for some amount of time (how long?) and then rename it back to __tuple? Are you suggesting that we instead never reuse a name that has been used for a header in the past for a directory?

I know that renaming files to directories can cause issues, I myself have issues downstream because of this. But I think those are shortcomings of the systems we are using, and I have been unwilling to create complexity upstream because of that, so far. If this problem is shared by more than just me, it may be worth finding a solution for everyone, however never reusing a previous file name is not going to fly. I think the best we can do is probably wait for one release before doing the renaming back to __tuple.

mgorny added a comment.Dec 5 2022, 6:27 AM

Well, I'd prefer renaming them permanently but one release would be good enough for Gentoo. Alternatively, at least for private headers we could streamline this by using a distinct prefix/suffix for directories, e.g. __tuple for a file, and __tuple_dir for directory. This would probably be cleaner than temporarily appending 2.

Mordante added inline comments.Dec 5 2022, 8:20 AM
libcxx/include/CMakeLists.txt
521

Not per se, if there are no complains from users I don't feel we need to.

mgorny added a comment.Dec 8 2022, 8:51 PM

Gentle ping. Do you have any specific requests on how this could be improved? I'd love to see the problem fixed before the next weekend (that's when I do snapshots for Gentoo).

I've hit this issue too and it was a source of frustration. Though the directory name is an internal detail that we should be free to change, I don't like the name __tuple2 as a permanent replacement. Unfortunately, any name we choose will be inconsistent with the names used for other such detail directories; the only directory currently present there that doesn't map directly to a header name is __debug_utils.

The core problem is that the install step does not attempt to delete any existing __tuple file before attempting to create the __tuple directory and copy files. Can we not just add a step to the install target to force that to happen?

The core problem is that the install step does not attempt to delete any existing __tuple file before attempting to create the __tuple directory and copy files. Can we not just add a step to the install target to force that to happen?

That won't help with anything that uses DESTDIR/some staging area before merging to the live filesystem, unfortunately.

Is __tuple2 acceptable to you if it's only for a single release?

I've hit this issue too and it was a source of frustration. Though the directory name is an internal detail that we should be free to change, I don't like the name __tuple2 as a permanent replacement. Unfortunately, any name we choose will be inconsistent with the names used for other such detail directories; the only directory currently present there that doesn't map directly to a header name is __debug_utils.

I ran into it in the past too, but as libc++ developer it's quite easy to figure out what happened. But I agree that for the average user the current way isn't user friendly.

The core problem is that the install step does not attempt to delete any existing __tuple file before attempting to create the __tuple directory and copy files. Can we not just add a step to the install target to force that to happen?

I like this idea better than using (temporary) __fooN headers. @philnik do you know how easy it is to remove these "obsolete" files? This would solve upgrading, but not downgrading from the current __foo is a directory to __foo is a file.

I like this idea better than using (temporary) __fooN headers. @philnik do you know how easy it is to remove these "obsolete" files? This would solve upgrading, but not downgrading from the current __foo is a directory to __foo is a file.

Like I said, it can't work. It implies that the build system would interfere with the live system and disrespect DESTDIR and friends. It would not work for distributions at all.

Is __tuple2 acceptable to you if it's only for a single release?

I don't think so. Unless I'm misunderstanding something, the same issue would occur when trying to revert to the original __tuple name since there is still no guarantee that a __tuple file from an earlier version wasn't still present.

That won't help with anything that uses DESTDIR/some staging area before merging to the live filesystem, unfortunately.

I'm not sure I'm following. It sounds like you are suggesting there are two issues:

  1. As part of the install step performed by the LLVM build system, a __tuple file in the install location will interfere with the installation of the __tuple directory.
  2. If the target of the install step performed by the LLVM build system is a staging area, then a secondary non-LLVM process that copies/merges (portions of) the LLVM install to a live system might (or might not) encounter a problem copying/merging the __tuple directory if a file is present.

If I have that right, then my perspective is that the second problem is not one that should be solved by LLVM. If I don't have that right, please clarify the scenario.

thesamesam added a comment.EditedDec 9 2022, 11:02 AM

Is __tuple2 acceptable to you if it's only for a single release?

I don't think so. Unless I'm misunderstanding something, the same issue would occur when trying to revert to the original `__tuple` name since there is still no guarantee that a __tuple file from an earlier version wasn't still present.

It wouldn't, because:

  • going from 15->16, __tuple gets replaced by __tuple2 (and the package manager can remove __tuple)
  • going from 16->17, __tuple2 is removed, and __tuple is added (as a new directory, not a replacement of a file)

You can't really have libcxx installed in parallel with multiple versions (at least, Gentoo doesn't, and I'm not aware of anyone else doing this, as it's not really interesting to do it), so it ends up being a non-issue as a result.

You might have it being an issue upgrading older systems, but then it already is now, and at least we mitigated it for most people.

No guarantee, but way less likely, and then way less inconvenience for the 90% case.

The issue is package managers often can't handle replacing a symlink directly with a directory. By having a release in between, it becomes the same as a new file.

That won't help with anything that uses DESTDIR/some staging area before merging to the live filesystem, unfortunately.

I'm not sure I'm following. It sounds like you are suggesting there are two issues:

  1. As part of the install step performed by the LLVM build system, a __tuple file in the install location will interfere with the installation of the __tuple directory.
  2. If the target of the install step performed by the LLVM build system is a staging area, then a secondary non-LLVM process that copies/merges (portions of) the LLVM install to a live system might (or might not) encounter a problem copying/merging the __tuple directory if a file is present.

If I have that right, then my perspective is that the second problem is not one that should be solved by LLVM. If I don't have that right, please clarify the scenario.

  1. This would affect any tarball to be installed to a system (including e.g. cpack) because you can't represent that "delete the dir first" part in extraction. Build systems do not usually touch the live filesystem for this reason - it prevents any sort of staging area or packing. I don't know if LLVM produces any of these, but the fact that a few people have mentioned they've hit this outside of Gentoo means it's not specific to us at all.
  2. Even if something is not _technically_ within LLVM's purview, it's often nice to distributions to accommodate them if it's not a major inconvenience. That's what the discussion above mentions -- a lot of this is about making life easier by handling it upstream briefly. It's an unusual operation to do (changing a file to a directory) and it's hit an edge-case which somebody has asked for a temporary workaround for. As explained earlier in this comment, it's sufficient for almost-all hitting this to just have it for one release, because then it's no longer a "replacement", just a "new directory".

The issue is package managers often can't handle replacing a symlink directly with a directory. By having a release in between, it becomes the same as a new file.

Doesn't that assume that the intervening package is actually installed?

If we just ship a renamed directory now, what are the chances that package managers will know to modify their package to remove the __tuple file so that a future version of the package can reuse that name? The chances seem pretty high to me.

For tarball-based installations, a temporary rename doesn't seem to allow a return to the original name without some additional ability to delete the original file.

I empathize with the issue you are facing. I don't have a lot of experience with package managers, but my understanding is that most have some ability to run code ahead of time. If nothing else, a package could have a dependency on another package that only contains post-install cleanup steps; e.g., a package that installs nothing and then runs a cleanup step to remove the __tuple file. Is this not a viable option for Gentoo?

mgorny added a comment.Dec 9 2022, 9:45 PM

Package managers track all installed files, and remove files that are no longer installed by the specific package version. However, the removal happens *after* installing the new version.

Yes, Gentoo can workaround this, and every other distribution out there can also probably work around this. The problem is: why does everyone has to repeatedly add complex workarounds when this can be resolved here trivially, so that things "just work" for everyone?

Package managers track all installed files, and remove files that are no longer installed by the specific package version. However, the removal happens *after* installing the new version.

Thank you; that resolves my concern about package managers neglecting to remove outdated files.

Yes, Gentoo can workaround this, and every other distribution out there can also probably work around this. The problem is: why does everyone has to repeatedly add complex workarounds when this can be resolved here trivially, so that things "just work" for everyone?

That question can be turned around; why should software authors need to rename their source files to work around deficiencies in package managers?

Part of my reaction is due to dislike for the proposed names. It is somewhat common to tag a number on to the end of a file name to indicate a new version of some feature, often one that is incompatible or risky in some way. In this case, source files are just being reorganized; the interface isn't changing. The "2" is confusing in that respect. I'm not fond of "__tuple_dir" either as the "_dir" part is redundant and doesn't add any additional information.

I took a closer look at the files in the "tuple" directory. None of them are directly included by the "tuple" header. They all appear to be related to support for tuple-like types in general; not to the implementation of std::tuple. That suggests names like "tuple_protocol", "tuple_support", or "tuple_like" might be more appropriate. If @ldionne is supportive of any of those names (or something similar), I'll have no objection to a rename.

tahonermann added inline comments.Dec 12 2022, 10:04 AM
libcxx/utils/generate_iwyu_mapping.py
34–37

This is a change I would object to on the basis that it is fragile.

I took a closer look at the files in the "tuple" directory. None of them are directly included by the "tuple" header. They all appear to be related to support for tuple-like types in general; not to the implementation of std::tuple. That suggests names like "tuple_protocol", "tuple_support", or "tuple_like" might be more appropriate. If @ldionne is supportive of any of those names (or something similar), I'll have no objection to a rename.

Interesting. Normally we always should include the __foo/*.h headers in foo. But here we indeed don't do that. I leave the choice to @ldionne.

I took a closer look at the files in the "tuple" directory. None of them are directly included by the "tuple" header. They all appear to be related to support for tuple-like types in general; not to the implementation of std::tuple. That suggests names like "tuple_protocol", "tuple_support", or "tuple_like" might be more appropriate. If @ldionne is supportive of any of those names (or something similar), I'll have no objection to a rename.

Interesting. Normally we always should include the __foo/*.h headers in foo. But here we indeed don't do that. I leave the choice to @ldionne.

I'm quite certain this was an oversight. Almost all the headers are actually used in <tuple>. I wonder why the modules build doesn't complain.

mgorny updated this revision to Diff 483821.Dec 18 2022, 6:56 AM
mgorny retitled this revision from [libc++] Rename __tuple to __tuple2 to avoid file collision to [libc++] Rename __tuple to __tuple_dir to avoid file collision.
mgorny edited the summary of this revision. (Show Details)

Use __tuple_dir instead of the icky __tuple2.

That question can be turned around; why should software authors need to rename their source files to work around deficiencies in package managers?

Well, I'm of the opinion that if you can help someone and it doesn't cost you anything, then there's no reason not to help. And that working with people who show concern about downstream problems and appreciation about their work is much nicer. The alternative is insulting distributors, so they lose interest, stop reporting bugs, start accumulating local patches and next thing you know, everything's one huge mess where every single program needs to include hacks for every variant of iibc++ out there.

That question can be turned around; why should software authors need to rename their source files to work around deficiencies in package managers?

Well, I'm of the opinion that if you can help someone and it doesn't cost you anything, then there's no reason not to help. And that working with people who show concern about downstream problems and appreciation about their work is much nicer. The alternative is insulting distributors, so they lose interest, stop reporting bugs, start accumulating local patches and next thing you know, everything's one huge mess where every single program needs to include hacks for every variant of iibc++ out there.

+1 I think we should try to avoid breaking downstream.

I see the build failed, this has nothing to do with your patch. Can you rebase your patch on main to fix that issue.

Thinking about it more, probably the cleanest long term solution would be to simply use .h suffix for files but that obviously won't solve the existing upgrade problem.

Would __tuple_helpers or __tuple_utils be less ugly resp. obvious solutions to the challenge?

Would __tuple_helpers or __tuple_utils be less ugly resp. obvious solutions to the challenge?

I'm open to any name. I'd just appreciate if someone made a decision, so I wouldn't be updating the patch a few times in a row.

That question can be turned around; why should software authors need to rename their source files to work around deficiencies in package managers?

This, x1000.

However, concretely I think it's reasonable to use this workaround if it's only for a single release, and then we can rename it to __tuple again. And indeed, if we follow our own conventions going forward, this shouldn't be an issue since detail headers have a .h extension.

Let's use __tuple_dir for LLVM 16 and rename it back to __tuple in LLVM 17. We also have the same problem for __string unless I am mistaken, @mgorny
can you handle that in your patch?

@Mordante @philnik We had agreed to include all __foo/*.h headers inside <foo>, but we didn't do it consistently and it turns out that it's sometimes impossible to do it consistently because of circular dependencies (in type_traits for example IIRC). We will have to reconsider that policy.

@mgorny Please let us know whether the above solution (i.e. rename but only for one release) works for you.

Let's use __tuple_dir for LLVM 16 and rename it back to __tuple in LLVM 17. We also have the same problem for __string unless I am mistaken, @mgorny
can you handle that in your patch?

Will do.

@mgorny Please let us know whether the above solution (i.e. rename but only for one release) works for you.

It's not perfect but it's good enough. I suppose you'll also accept future one-release renames if similar problems occur?

Also, I'd really prefer if you didn't call it "deficiency in the package manager" because I find that insulting to the lot of effort many of us put in the package managers. It's not a "deficiency", it's a conscious decision not to add a lot of complexity combined with fragility to handle a corner case affecting at most a few (libc++ is the only one I'm aware of) out of 20000+ packages.

We also have the same problem for __string unless I am mistaken, @mgorny

Actually, __string was granularized in LLVM 15 already, so I don't think that's necessary. However, I can still do it if you preferred.

We also have the same problem for __string unless I am mistaken, @mgorny

Actually, __string was granularized in LLVM 15 already, so I don't think that's necessary. However, I can still do it if you preferred.

Oh, really? Ok, then leave it out.

It's not perfect but it's good enough. I suppose you'll also accept future one-release renames if similar problems occur?

Yes, in fact the expectation would be that we rename them to __foo_dir immediately upon creating them if we remember about this (which I hope we will, and also the risk for this happening again is not suuuuper high since we don't have that many headers in that category).

Also, I'd really prefer if you didn't call it "deficiency in the package manager" because I find that insulting to the lot of effort many of us put in the package managers. It's not a "deficiency", it's a conscious decision not to add a lot of complexity combined with fragility to handle a corner case affecting at most a few (libc++ is the only one I'm aware of) out of 20000+ packages.

Don't get me wrong, I think everybody here has tremendous respect for folks who handle distributions. From our perspective though, it seems kind of surprising that making a seemingly valid and simple change to our code organization would trigger an issue much further down the distribution pipeline due to a directory/file mismatch. But like I said, I don't think it matters much -- the workaround is simple and temporary, so we'll try to remember to do it on a best effort basis and everyone should be happy.

libcxx/utils/generate_iwyu_mapping.py
34–37

I actually don't quite understand why this change is needed, can you explain? Sorry if that's simple.

mgorny added inline comments.Dec 21 2022, 7:10 AM
libcxx/utils/generate_iwyu_mapping.py
34–37

Without this change:

diff --git a/libcxx/include/libcxx.imp b/libcxx/include/libcxx.imp
index 2c591b71737b..2ed026d76e4a 100644
--- a/libcxx/include/libcxx.imp
+++ b/libcxx/include/libcxx.imp
@@ -39,7 +39,7 @@
   { include: [ "@<__string/.*>", "private", "<string>", "public" ] },
   { include: [ "@<__support/.*>", "private", "<support>", "public" ] },
   { include: [ "@<__thread/.*>", "private", "<thread>", "public" ] },
-  { include: [ "@<__tuple_dir/.*>", "private", "<tuple>", "public" ] },
+  { include: [ "@<__tuple_dir/.*>", "private", "<tuple_dir>", "public" ] },
   { include: [ "@<__type_traits/.*>", "private", "<type_traits>", "public" ] },
   { include: [ "@<__utility/.*>", "private", "<utility>", "public" ] },
   { include: [ "@<__variant/.*>", "private", "<variant>", "public" ] },
ldionne added inline comments.Dec 21 2022, 7:58 AM
libcxx/utils/generate_iwyu_mapping.py
34–37

Oh, thanks. I suggest we do this instead:

temporary_mappings = {'__tuple_dir': 'tuple'}
for dir in detail_directories:
    public_header = temporary_mappings.get(dir, dir.lstrip('_'))
    result.append(f'{generate(f"@<{dir}/.*>", public_header)},')

That way we'll find this when we grep for __tuple_dir in the future.

mgorny updated this revision to Diff 484586.Dec 21 2022, 8:07 AM

Use explicit mapping as requested.

mgorny marked 3 inline comments as done.Dec 21 2022, 8:07 AM
mgorny added inline comments.
libcxx/utils/generate_iwyu_mapping.py
34–37

Thanks. I've done that, except that I've left i as the loop var since dir is a built-in function in Python.

ldionne accepted this revision.Dec 21 2022, 8:23 AM

Thanks, LGTM with green CI.

This revision is now accepted and ready to land.Dec 21 2022, 8:23 AM
This revision was automatically updated to reflect the committed changes.
mgorny marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 10:21 AM