Absolute2

[mzuenni]

No description provided.

[mzuenni]

@RagnarGrootKoerkamp I closed the other PR since it was really outdated... this should use absolute where it is ok

[RagnarGrootKoerkamp]

oh I guess this is not a stacked diff but a diff containing all the other PRs into draft as well.

anyway lgtm i gues

[RagnarGrootKoerkamp]

What was the original reason for preferring absolute paths again exactly?

#356 mentions that .absolute() avoids resolving symlinks, but why was that important again?
(I guess if we do x.resolve()/../blah, then it's better to not acidentally go through the symlink?

[mpsijm]

I'm not sure, actually 😅 I think it's difficult to test the implications of this, I don't have a mental list of all the ways how symlinks are used in BAPCtools. Also, we may want to check if log/warn/error messages don't get blown up with full absolute paths now, in cases where shorter relative paths would suffice.

Also, I don't think this really belongs to the draft PR. That said, it's also currently not a good idea to re-do this on master, because in draft, we have been changing lots of code that uses these paths. 🙃

[RagnarGrootKoerkamp]

Most error reporting already makes paths relative to elsewhere.

Thinking of it, that may be the reason why this is a good change: given two absolute paths it's much more reliable to the relative version from one to the other.
Especially with .resolve(), it can be that one of the paths had a symlink and the inferred relative path is not what we'd want it to be.

After this change, we can probably have a problem directory that's a symlink to another place and it should behave more consistently as if the symlink wasn't there at all?

[mzuenni]

I think the only reason why we want to change this is if the problem itself is symlinked somewhere else? Then resolve would resolve the path to the problem which is weird from a user perspective?

[mpsijm]

Can we:

• For the remaining instances of .resolve(), add a comment why .absolute() is not used there (or simply change them to .absolute(), if possible)?
• Add a pre-commit hook that prevents the use of .resolve() in the future, with @mzuenni's explanation as message when the hook fails? Can be as simple as a ! grep "\.resolve()" **/*.py. Of course, we should have some way to suppress the warning with an explanatory comment.

[RagnarGrootKoerkamp]

May be a bit over the top to have a precommit hook, but that stuff is fast anyway so why not 🤷

[mpsijm]

I just think it would be a shame if this decision gets lost/forgotten in the future, when nothing is preventing the use of .resolve() 😛 And if the check is automatic, we don't have to pay attention to it during reviews 😄

[mzuenni]

Not sure how to add such a hook... and how to handle those three cases where resolve is still used. ^^'

• 2 of them "read the link" in that case resolve should be used
• the third checks if something is in the tmp_dir, since we never build links to that dir absolute would be fine but IMO semantically checking this with resolve makes more sense