refactor: split slicer into steps and remove unnecessary state #136
Conversation
letFunny
force-pushed
the
refactor-slicer-steps
branch
from
5169ba3
to
e207761
Compare
letFunny
force-pushed
the
refactor-slicer-steps
branch
from
ad0bb03
to
9905560
Compare
letFunny
force-pushed
the
refactor-slicer-steps
branch
from
9905560
to
6b60e86
Compare
internal/slicer/slicer.go
Outdated
| @@ -72,33 +83,33 @@ func Run(options *RunOptions) (*Report, error) { | |||
| targetDirAbs = filepath.Join(dir, targetDir) | |||
| } | |||
|
|
|||
| packages, err := fetchPackages(options) | |||
There was a problem hiding this comment.
[Comment for reviewer]: Before, we were doing package fetching in two steps, checking if the package existed and then fetching it.
It makes sense for the refactor to consolidate both for the sake of readability but, apart from that reason, we also need this function for the chisel.db and for the FIPS PRs. The reason for the former is that we need a single place were we have all the data about packages and archives in order to include that information in the db. The reason for the latter is that we have to do more logic for archive selection and it is far simpler if that logic is all in one place.
| @@ -482,7 +482,7 @@ func (s *S) TestExtractCreateCallback(c *C) { | |||
| relPath = relPath + "/" | |||
| } | |||
| sort.Slice(extractInfos, func(i, j int) bool { | |||
| return strings.Compare(extractInfos[i].Path, extractInfos[j].Path) < 0 | |||
| return extractInfos[i].Path < extractInfos[j].Path | |||
There was a problem hiding this comment.
[Comment for reviewer]: Per the go compile source code:
// NOTE(rsc): This function does NOT call the runtime cmpstring function,
// because we do not want to provide any performance justification for
// using strings.Compare. Basically no one should use strings.Compare.
internal/slicer/slicer.go
Outdated
| extractPackage = make(map[string][]deb.ExtractInfo) | ||
| extract[slice.Package] = extractPackage | ||
| } | ||
| arch := archives[slice.Package].Options().Arch | ||
| archiveName := options.Selection.Release.Packages[slice.Package].Archive |
There was a problem hiding this comment.
[Comment for reviewer]: The archive map from archive name to archive was redundant and was only used to get the architecture. When we add support for FIPS we will return the architecture as part of the package data but for now I prefer to keep it like this.
letFunny
added
Simple
internal/slicer/slicer.go
Outdated
| continue | ||
| } | ||
| if done[relPath] || pathInfo.Kind == setup.CopyPath || pathInfo.Kind == setup.GlobPath { | ||
| continue | ||
| } | ||
| done[relPath] = true | ||
| addKnownPath(relPath) | ||
| addParents(knownPaths, relPath) |
There was a problem hiding this comment.
I've been looking at this PR for a while, but I have to say I'm quite uncomfortable with the diff as is. It's not that it is wrong, but rather that it is difficult to understand if it is wrong or not. Major pieces are being moved around, which was the plan, but also once again we have semantic changes that were not supposed to be bundled with a major move, as it makes reviewing hard. For example, here we used to add a known path and all its parents, but now we're simply adding parents. Why? Elsewhere we have a reviewer note claiming we now do not trust parents to have been added because maybe they weren't, but that was an impossible situation on the current design because we always add known paths together with their own parents, thus every time you hit a parent you know their preceding parents have also been added.
Can we please take a step back and do as we discussed before: major code reorganizations should be done independently from semantic changes, otherwise we can easily introduce hard to track bugs as has happened recently elsewhere.
Then, there are minor-looking changes that are actually bundled here for no good reason. For example, it modifies where the architecture setting is looked at. Why? How's that related to splitting slicer steps?
We need to take a step back here. If this is about moving major parts of some complex logic around, let's please not touch anything else while doing this, and let's try to preserve what is being done as much as possible, so our brains can process this in "same thing but elsewhere" mode. If you want to change other details later, that's fine of course, but then we'll have a chance of reviewing the changes as actual changes.
There was a problem hiding this comment.
Per the discussion we had today I will remove all the changes to the logic, namely:
internal/slicer/slicer.go
Outdated
|
|
||
| // removeUntilMutate removes entries marked with until: mutate. A path is marked | ||
| // only when all slices that refer to the path mark it with until: mutate. | ||
| func removeUntilMutate(rootDir string, knownPaths map[string]pathData) error { |
There was a problem hiding this comment.
s/removeUntilMutate/removeAfterMutate/
At the moment it's reading reversed: it's actually removing now that files have already been mutated, instead of before (and until) they are mutated.
| CheckWrite: checkWrite, | ||
| CheckRead: checkRead, | ||
| CheckWrite: checker.checkMutable, | ||
| CheckRead: checker.checkKnown, |
There was a problem hiding this comment.
Not sure we want to break away from the original names, but we can roll with that for now and see how far we get.
Streamline
slicer.gomoving all of the non essential functionality to self-contained function / abstractions and adding comments where necessary. Most importantly, combine all maps that recorded information about paths into one (knownPaths,pathInfosandpathUntil) and record the information in the same place instead of during several stages.