Cleanup: move internal modules in subdirectories and remove/integrate util scripts #162
Conversation
|
Biggest change in component code to resolve any backwards incompatible issues will be to import I'd like to get master working on all repos without any backports for incompatibility; only after that stage, i can readd e.g. |
src/main/bin/ccm-fetch
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/perl -w | |||
| #!/usr/bin/perl ## no critic ((RequireVersionVar); | |||
There was a problem hiding this comment.
I am not in favour of having to modify every single file with this per-line opt-out. Why not just disable the policy globally in PerlCritic? It's only activated at level 2 (the second highest) and perlcritic tends to get annoying and less useful at level 3, so I can't see us ever wanting to enable level 2 policies.
There was a problem hiding this comment.
this workaround is only needed for scripts (so i think 4 or 5 in all repos); and this is a bug in critic, see Perl-Critic/Perl-Critic#711
There was a problem hiding this comment.
That makes me even less keen on it - it should only be needed temporarily and no one will be brave enough to remove it!
src/main/bin/ccm-initialise
Outdated
| @@ -127,7 +127,11 @@ MakeCacheRoot($this_app, $cache_root, $dopts); | |||
|
|
|||
| # make global lock file (mask and GID take care of the permissions) | |||
| $this_app->verbose("Creating lockfile"); | |||
| <<<<<<< a5c81c552251e536424fd3de83ed2560cb70e952 | |||
There was a problem hiding this comment.
Looks like you've accidentally checked in a merge error?
|
@ned21 it's not that easy, it changes history from another PR if i do this. i'll clean this up once the other PRs are merged. |
stdweird
force-pushed
the
more_cleanup
branch
from
a63c624
to
e322ef2
Compare
|
@ned21 rebased |
| # full directory path, and $ele_path is the element path (as string) | ||
| # | ||
| sub _resolve_eid | ||
| sub _resolve_enc_eid |
There was a problem hiding this comment.
This seems to rename every instance of eid to enc_eid. However isn't eid already short for "encoded id" -- in which case this is now a double encoded id? Is the enc added for clarity? In which case, should it be _resolve_enc_id ?
There was a problem hiding this comment.
can you reread the pod in Encode.pm to see if it makes any sense at all? (it should also explain what an eid is (and it's not "encoded id", it's probably short for euhm, index? which is the response to "and how should we abbreviate this path counter?" 😄 )
There was a problem hiding this comment.
OK, pod in Encode.pm refers to "encoded eid" so I guess this is all consistent.
|
Code looks OK except for the one question above. Now it's all rebased, is there a reason 3971e5c can't be squashed into the commit that introduced it? |
|
@ned21 i have to look into the the squashing. it's a commit in the master, i'm afraid this will involve a |
|
I think you have a conflict with master but the conflict resolution in 10bfd63 is wrong (it results in things that are Not Perl). Sometimes git remembers previous conflict resolutions which can make it hard to redo them but if you squash the later commit into 10bfd63 then I think everything will be fine. |
stdweird
force-pushed
the
more_cleanup
branch
5 times, most recently
from
221fb0a
to
ca26a2f
Compare
|
It's now a lot less backwards incompatible. the |
|
Discussed at March 2017 workshop, no issues with the code but the first commit adds a set of merge conflict markers, which is very ugly. |
|
@ned21 both commits deleted |
…ainly for pre 1.51 maven-tools
This was missed when CCM was restructured in quattor#162 (merged in 1713a03).
Based on #160 and #156 and new build-tools with quattor/maven-tools#121
Solving issues due to backwards incompatibility should not require the new release, 16.10 is sufficient