install_symlink: Handle $DESTDIR case for links with absolute path #10176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10176 +/- ##
==========================================
- Coverage 66.14% 66.11% -0.03%
==========================================
Files 406 406
Lines 86634 86640 +6
Branches 19161 19157 -4
==========================================
- Hits 57305 57283 -22
- Misses 24897 24934 +37
+ Partials 4432 4423 -9
Continue to review full report at Codecov.
|
In case a link is pointing_to an absolute path and we are using $DESTDIR we fail in case the target is missing. This is incorrect because we may need to use an absolute path to an already installed file that is in $DESTDIR. So if an absolute target is not existing, check if we have such file in $DESTDIR before failing for real.
3v1n0
force-pushed
the
symlinks-destdir
branch
3 times, most recently
from
519be99
to
e06fc61
Compare
Please don't. We already offer a battle-tested high level abstraction for this, while using pathlib pays the cost of the pathlib abstraction and makes for an abstraction of an abstraction while... simultaneously solving none of the ugliness of string slicing. |
Well, the comment on the code was mentioning the idea of using it, so I tried. But I'm happy to revert if there's no consesus. |
|
Well, "consider". :p As it turns out, after consideration I don't think pathlib has a straightforward way to do what actually turns out to not be path-like semantics (at least as pathlib defines it). And it shows in the fact that the proposed implementation still has to drop back down to strings and do string slicing. Anyway, the first patch is useful on its own and seems correct. Thanks. |
|
@eli-schwartz ok I've dropped the 2nd commit, I can propose it in another PR in case. |
| @@ -431,10 +431,12 @@ def do_copyfile(self, from_file: str, to_file: str, | |||
| append_to_log(self.lf, to_file) | |||
| return True | |||
|
|
|||
| def do_symlink(self, target: str, link: str, full_dst_dir: str, allow_missing: bool) -> bool: | |||
| def do_symlink(self, target: str, link: str, destdir: str, full_dst_dir: str, allow_missing: bool) -> bool: | |||
There was a problem hiding this comment.
Actually I wonder. We use both target and link, but we don't "use" full_destdir, we just need it to define the abs_target. I am not sure whether here or at the call site is maybe a better place to calculate this?
(I genuinely am not sure.)
There was a problem hiding this comment.
I was thinking about that, but full_dst_dir is still used by parent to create the directory,
We could definitely do that under symlink, but we'd still need to bring the DirMaker with us
|
Wondering: In such cases, and so when the absolute path is something that meson has installed, shouldn't we try to convert the symlink to be relative instead? |
|
I would say in such a case, if the project developers specified an absolute path how do we know they want it to be turned into a relative one? Probably the only case that would ever be true, they really want |
|
CI failure on Cygwin is caused by gcovr/gcovr#576 -- yippee for strange Windows envs that cannot be baked into a CI image which is regularly updated and tested before being released as a new frozen image. Not only is it slow to repeatedly install everything, it also periodically changes the versions of everything installed, then breaks sometimes. ;) All the rest of the CI passes, and the breakage is known and shortly to be fixed. Merging. |
In case a link is pointing_to an absolute path and we are using $DESTDIR
we fail in case the target is missing.
This is incorrect because we may need to use an absolute path to an
already installed file that is in $DESTDIR.
So if an absolute target is not existing, check if we have such file in
$DESTDIR before failing for real.