Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the notifier module to all submodules that are instrumented #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

herzogp
Copy link
Contributor

@herzogp herzogp commented Oct 1, 2024

No description provided.

relFolders := []string{notifierRelPath}
someOffset = common.PathFromBaseDirectory(cfx.inputDirectory, modFolder)
if someOffset != "" {
num_parents := len(strings.Split(someOffset, pathSep))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably use https://pkg.go.dev/path#Split instead of strings. Just incase there is an escaped path seperator in a file/directory name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to know the number of directory levels to navigate to from modFolder, back to the antithesis module that will be required.

Example:
../../../../../notifier evaluated from /work/customer/aa/bb/cc/dd should resolve to /work/notifier

In this case, we need to know how many relative levels to 'go up', from the dd directory, to refer to the notifier module.

By using strings.Split (respecting the platform specific PathSeparator) we can readily get the number of levels needed.

Using path.Split() or filepath.Split() only parses the basename and the directory. We would have to repeatedly parse the directory to see how many levels there are. The hope is that filepath.Split() will properly handle filenames that contain PathSeparator characters. Is it worth it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	f, _ := filepath.Rel("/test/t/x/y", "/fd")
	fmt.Println(f)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: "tastes great, less filling"

someOffset = common.PathFromBaseDirectory(cfx.inputDirectory, modFolder)
if someOffset != "" {
	destModuleFolder := filepath.Join(cfx.customerDirectory, someOffset)
	os.MkdirAll(destModuleFolder, 0777)

	basePath := filepath.Join(cfx.customerDirectory, someOffset)
	targPath := cfx.notifierDirectory
	if altDestModuleFolder, erx := filepath.Rel(basePath, targPath); erx == nil {
		common.AddDependencies(modFolder, destModuleFolder, cfx.instrumentorVersion, notifierModule, altDestModuleFolder)
	}
}

for i := 0; i < num_parents; i++ {
relFolders = append(relFolders, notifierRelPath)
}
subRelPath := strings.Join(relFolders, pathSep)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ^^

@@ -258,6 +286,10 @@ func (cfx *CommandFiles) FindSourceCode() (paths []string, numSkipped int, err e
return nil
}
if !strings.HasSuffix(path, ".go") {
if strings.HasSuffix(path, "go.mod") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pkg.go.dev/path#Base

should probably be filepath.Base(path)=="go.mod"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - uses filepath.Split() to get both dir and baseFile, then compares baseFile to "go.mod" and if they are equivalent, will consider dir as containing a submodule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -258,6 +286,10 @@ func (cfx *CommandFiles) FindSourceCode() (paths []string, numSkipped int, err e
return nil
}
if !strings.HasSuffix(path, ".go") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -312,3 +345,37 @@ func (cfx *CommandFiles) CreateSymbolTableWriter(filesHash string) (symWriter *i
func (cfx *CommandFiles) GetNotifierDirectory() string {
return cfx.notifierDirectory
}

func (cfx *CommandFiles) ShowDependentModules() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these functions are not recursive. Can dependent modules have recursive dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependent modules can not have recursive dependencies

destGoModFile := fmt.Sprintf("%s/go.mod", customerOutputDirectory)
localNotifier := fmt.Sprintf("../%s", NOTIFIER_FOLDER)
localNotifier := filepath.Join(notifierRelPath, NOTIFIER_FOLDER)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return ""
}
someOffset := someNorm
if strings.HasPrefix(someNorm, baseNorm) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be a can of worms. imagine the following scenario:

/path/to/dir
/path/to/dir_xyz

the second one will look like the prefix of the first one even though it is not a subdirectory. This is a non-trivial thing to test for (see golang/go#18358). Probably the simplest way that works 99% of the time is:

filepath.match(filepath.join(baseNorm,"*"), someNorm)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants