CAF::Path: add methods to manage links

jouvin commented 7 years ago

3 methods added to manage symlinks:

is_symlink(): returns true if the argument is a symlink
symlink(): creates a new symlink or updates an existing one. If the symlink path already exists and is not a symlink, returns an error.
hardlink(): same as symlink() for hardlinks.
has_hardlinks(): test if a file has hardlinks (several entries refering to the same inode)
is_hardlink(): compare to paths and check if they refer to the same inode
- Note that there it is impossible to say if a file is a hardlink in itself... A hardlink is just a normal file.

stdweird commented 7 years ago

@jouvin please have a look at eg the directory code. try to use the wrapped LC calls or existing CAF::Path methods. there has to be support for NoAction and there have to be unittests for NoAction

jouvin commented 7 years ago

@stdweird thanks for all the remarks, I'll try to improve the code later today...

jouvin commented 7 years ago

@stdweird @jrha @ned21 any opinion on the argument order (link path versus target)? Perl symlink is following ln command with target first, link second. LC::Check::symlink is following the order I had in mind initially: link first, target second. As CAF::Path::symlink is mainly a wrapper over LC::Check::symlink, I'm inclined to follow its order but I don't have a strong opinion. And I am happy to change if you think the Perl symlink order is less confusion/more appropriate...

jouvin commented 7 years ago

Reimplemented over LC::Check:link() as suggested by @stdweird. Some problems in tests, to be fixed later...

ned21 commented 7 years ago

@jouvin - tough question! Following LC::Check::symlink makes it easier to refactor code in the short term but long term following symlink() is better for everyone and less confusing. Since I think we want to take a long term view, I vote for following the Perl and UNIX symlink ordering.

jouvin commented 7 years ago

I think the code is now in pretty good shape. We may just need to adjust the argument order and in fact part of the refactoring we want to do after having these methods is replacing symlink by CAF::Path. For this @ned21 proposal is probably less confusing, I agree...

I am still struggling with the tests: @stdweird verify_exception() is failing and I don't really understand what it is supposed to do (and I have not seen any comment in the code to help with it!). Any explanation much appreciated! Once tests are working for symlink(), I still need to add tests for hardlink() and tests for NoAction but it should be easy.

jouvin commented 7 years ago

@stdweird see https://github.com/quattor/CAF/pull/225#issuecomment-284270389, I'm still interested by a short explanation about verify_exceptions() and what it is supposed to do...

jouvin commented 7 years ago

Should be really better now (as usual!) 😄 The following problems have been addressed:

Argument order is the same as symlink
_safe_eval() has been fixed: for me, the way to detect failures were only detecting run time errors, not functions returning a failed status (undef). See eval docucmentation. As a result, $self->{fail} was not set. New code is working as expected with all the previously existing tests. @stdweird please validate! (this is in a separate commit).

I managed to understand how init_exception()/verify_exception() work! Great work in fact!

Still to be done in unit tests:

unit tests for NoAction
Check the result of actions is what it is said to be by checking that the object is really created or really not created/updated.

I'd like to get feedback on what is available first...

jouvin commented 7 years ago

Previous remarks addressed. Still missing tests for NoAction.

jouvin commented 7 years ago

Tests should be complete now! At least ready for detailed scrutiny by @stdweird !

jouvin commented 7 years ago

has_hardlinks() and is_hardlink() added as they were basically needed for the unit tests... PR should really be complete now!

jouvin commented 7 years ago

@ned21 @jrha @stdweird Adding more unit tests for hardlinks (in particular hardlinks to symlinks, something allowed by ln command whatever the symlink target is), I found 2 bugs in in LC::Check::symlink. See https://github.com/quattor/LC/issues/17. Basically we have the choice between fixing LC::Check (easy fix, see https://github.com/quattor/LC/issues/16) or preventing some (probably marginal) use cases in CAF::Path. What do you think?

jouvin commented 7 years ago

PR should be almost ok but I still have to figure out the problem with verify_exceptions(). Role of the noreset argument is unclear for me. @stdweird could you give me some info?

jouvin commented 7 years ago

I realized that we had a test-specific version of LC::Check (and Process and Files). It caused some tests to fail because the version of the module was not advertized but CAF::Path had a requirement on it. It is unclear for me why this was happening for certains tests only (seems to be those for modules using CAF::Path, except CAF::Path itself). Do we really need these test-specific versions of LC modules. Just removing them is unfortunately not working, so there are probably some subtilties...

jouvin commented 7 years ago

Ready for final review 😄

stdweird commented 7 years ago

@jouvin more minor fixes. can you address them in a separate commit(s)? you can squash them before merging (it would make the review easier)

quattor / CAF

CAF::Path: add methods to manage links #225