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

[RSDK-9498] Search for meta.json File in Top Level of Online Module #4689

Merged
merged 50 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
974351e
pass directories
bashar-515 Jan 2, 2025
d15c8c1
search through top-level directory
bashar-515 Jan 3, 2025
249ff76
restore manager.go file
bashar-515 Jan 3, 2025
9fac490
actually restore manager.go file
bashar-515 Jan 3, 2025
19287f3
create helper function
bashar-515 Jan 3, 2025
4b46142
use environment variable if registry module
bashar-515 Jan 3, 2025
91ece93
remove print statement
bashar-515 Jan 3, 2025
df1742d
fix lint errors
bashar-515 Jan 3, 2025
49efade
restore manager.go file
bashar-515 Jan 6, 2025
4a86d33
restore manager.go file
bashar-515 Jan 8, 2025
08e0757
always return top level directory
bashar-515 Jan 8, 2025
a320e8a
always use helper function to find meta.json file
bashar-515 Jan 8, 2025
56e4222
reword error returned when no meta.json file found
bashar-515 Jan 8, 2025
a757e69
check for non-nil error first
bashar-515 Jan 8, 2025
947fd2f
rework order of conditions
bashar-515 Jan 8, 2025
a2b6fbe
update comment
bashar-515 Jan 8, 2025
79db09d
update TODO comment
bashar-515 Jan 8, 2025
0524189
fix numbering typo in comment
bashar-515 Jan 8, 2025
159b723
correctly return error
bashar-515 Jan 8, 2025
7797a34
lint
bashar-515 Jan 9, 2025
ce8f7b9
fix typo
bashar-515 Jan 9, 2025
a4a0a9c
add unit tests for finding meta.json file
bashar-515 Jan 9, 2025
c0ed578
export previously added unit test
bashar-515 Jan 9, 2025
242c644
correctly check error type in unit tests
bashar-515 Jan 9, 2025
06382f4
reword errors
bashar-515 Jan 10, 2025
1652778
include module type in errors
bashar-515 Jan 10, 2025
ac9380a
clarify cases
bashar-515 Jan 10, 2025
3732b8a
remove unnecessary conditions
bashar-515 Jan 10, 2025
581f6b8
begin implementing unit tests
bashar-515 Jan 10, 2025
33ae953
write unit tests for registry module case
bashar-515 Jan 13, 2025
e5f6122
test local non-tarball case
bashar-515 Jan 13, 2025
5666808
fix case description
bashar-515 Jan 13, 2025
20fc522
mark test as complete
bashar-515 Jan 13, 2025
a97e648
correctly create error
bashar-515 Jan 13, 2025
a9f1ff9
lint
bashar-515 Jan 13, 2025
b2318af
finish unit tests for getJSONManifest()
bashar-515 Jan 13, 2025
e700d0f
update comments
bashar-515 Jan 13, 2025
5502231
write FirstRun() unit tests
bashar-515 Jan 13, 2025
6d50344
remove unused helper function
bashar-515 Jan 13, 2025
a3a79ed
remove unused import
bashar-515 Jan 13, 2025
995fd41
lint
bashar-515 Jan 13, 2025
1cec229
check logs
bashar-515 Jan 13, 2025
a9360d6
only search unpacked module directory if different from top level mod…
bashar-515 Jan 13, 2025
7498514
add comments explaining environment variables
bashar-515 Jan 15, 2025
c6b66b1
rename test function to specify that it only tests registry modules
bashar-515 Jan 15, 2025
6c8f702
create helper function
bashar-515 Jan 15, 2025
f00fc03
lint
bashar-515 Jan 15, 2025
6698cfc
add new tests
bashar-515 Jan 15, 2025
99e3305
lint
bashar-515 Jan 15, 2025
52656ca
remove TODO comment
bashar-515 Jan 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 107 additions & 31 deletions config/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bufio"
"context"
"encoding/json"
stderrors "errors"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -267,7 +268,7 @@ func (m *Module) FirstRun(

// Load the module's meta.json. If it doesn't exist DEBUG log and exit quietly.
// For all other errors WARN log and exit.
meta, err := m.getJSONManifest(unpackedModDir)
meta, moduleWorkingDirectory, err := m.getJSONManifest(unpackedModDir, env)
Copy link
Member

Choose a reason for hiding this comment

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

want to see an integration test around this whole function

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean around getJSONManifest()? or around the FirstRun() which calls getJSONManifest()?

Copy link
Member

Choose a reason for hiding this comment

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

around FirstRun() at the very least, would recommend trying to unit test getJSONManifest() as well

var pathErr *os.PathError
switch {
case errors.As(err, &pathErr):
Expand All @@ -282,7 +283,7 @@ func (m *Module) FirstRun(
logger.Debug("no first run script specified, skipping first run")
return nil
}
relFirstRunPath, err := utils.SafeJoinDir(unpackedModDir, meta.FirstRun)
relFirstRunPath, err := utils.SafeJoinDir(moduleWorkingDirectory, meta.FirstRun)
if err != nil {
logger.Errorw("failed to build path to first run script, skipping first run", "error", err)
return nil
Expand Down Expand Up @@ -377,42 +378,117 @@ func (m *Module) FirstRun(
return nil
}

// getJSONManifest returns a loaded meta.json from one of two sources (in order of precedence):
// 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case.
// 2. if this is a local tarball and there's a meta.json next to the tarball, use that.
// TODO(RSDK-9498): write test(s)
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

// getJSONManifest returns a loaded meta.json from one of three sources (in order of precedence):
// 1. if this is a registry module and there is a meta.json in its top level directory, use that.
// 2. if there is a meta.json in the exe dir, use that, except in local non-tarball case.
// 3. if this is a local tarball, use the meta.json in unpackedModDir.
// Note: the working directory must be the unpacked tarball directory or local exec directory.
func (m Module) getJSONManifest(unpackedModDir string) (*JSONManifest, error) {
// note: we don't look at internal meta.json in local non-tarball case because user has explicitly requested a binary.
localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage()
if !localNonTarball {
// this is case 1, meta.json in exe folder.
metaPath, err := utils.SafeJoinDir(unpackedModDir, "meta.json")
bashar-515 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

would like to see a test for this

// note: all registry modules qualify for cases 1 & 2; local tarballs for cases 2 & 3; and local non-tarballs for none. We don't look at
// internal meta.json in local non-tarball case because user has explicitly requested a binary.

// note: each case is exited iff no errors occur but the meta.json file is not found

var ok bool
var moduleWorkingDirectory string
var registryErr error

online := m.Type == ModuleTypeRegistry

// case 1: registry
if online {
moduleWorkingDirectory, ok = env["VIAM_MODULE_ROOT"]
benjirewis marked this conversation as resolved.
Show resolved Hide resolved
if ok {
var meta *JSONManifest
meta, registryErr = findMetaJSONFile(moduleWorkingDirectory)
if registryErr != nil {
// return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found
if !os.IsNotExist(registryErr) {
return nil, "", errors.Wrap(registryErr, "registry module")
}
}

if meta != nil {
return meta, moduleWorkingDirectory, nil
}
}
_, err = os.Stat(metaPath)
if err == nil {
// this is case 1, meta.json in exe dir
meta, err := parseJSONFile[JSONManifest](metaPath)
if err != nil {
return nil, err
}

var registryTarballErr error

localNonTarball := m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage()
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Realize you didn't write this, but I think the m.Type == ModuleTypeLocal is redundant with the NeedSyntheticPackage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it actually is necessary to strictly define the case. The only way I've been able to wrap my head around it is by writing out the boolean expressions defining the case and using an online boolean expression simplifier (lol) to, well, simplify them. As its written, case 2 is entered when the following expression evaluates to true.
A) case 2 = !localNonTarball = !m.Type == ModuleTypeLocal && !m.NeedsSyntheticPackage() = !(L(!(LT))) which simplifies to !L+T. So, the case is supposed to be registry or tarball. Now if we do the same thing without m.Type == ModuleTypeLocal:
B) case 2 = !localNonTarball = !!m.NeedsSyntheticPackage() = !(!(LT)) = LT, which is only modules that are both local and tarballs.


// case 2: registry OR tarball
if !localNonTarball && unpackedModDir != moduleWorkingDirectory {
var meta *JSONManifest
meta, registryTarballErr = findMetaJSONFile(unpackedModDir)
if registryTarballErr != nil {
if !os.IsNotExist(registryTarballErr) {
if online {
return nil, "", errors.Wrap(registryTarballErr, "registry module")
}

return nil, "", errors.Wrap(registryTarballErr, "local tarball")
}
return meta, nil
}

if meta != nil {
return meta, unpackedModDir, nil
}
}

var exeDir string
var localTarballErr error

// TODO(RSDK-7848): remove this case once java sdk supports internal meta.json.
// case 3: local AND tarball
if m.NeedsSyntheticPackage() {
// this is case 2, side-by-side
// TODO(RSDK-7848): remove this case once java sdk supports internal meta.json.
metaPath, err := utils.SafeJoinDir(filepath.Dir(m.ExePath), "meta.json")
if err != nil {
return nil, err
exeDir = filepath.Dir(m.ExePath)

var meta *JSONManifest
meta, localTarballErr = findMetaJSONFile(exeDir)
if localTarballErr != nil {
if !os.IsNotExist(localTarballErr) {
return nil, "", errors.Wrap(localTarballErr, "local tarball")
}
}
meta, err := parseJSONFile[JSONManifest](metaPath)
if err != nil {
// note: this error deprecates the side-by-side case because the side-by-side case is deprecated.
return nil, errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath)

if meta != nil {
return meta, exeDir, nil
}
}

if online {
if !ok {
return nil, "", errors.Wrap(registryTarballErr, "registry module: failed to find meta.json. VIAM_MODULE_ROOT not set")
}
return meta, err

return nil, "", errors.Wrap(stderrors.Join(registryErr, registryTarballErr), "registry module: failed to find meta.json")
}

if !localNonTarball {
return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json")
}

return nil, "", errors.New("local non-tarball: did not search for meta.json")
}

func findMetaJSONFile(dir string) (*JSONManifest, error) {
metaPath, err := utils.SafeJoinDir(dir, "meta.json")
if err != nil {
return nil, err
}

_, err = os.Stat(metaPath)
if err != nil {
return nil, err
}
return nil, errors.New("failed to find meta.json")

meta, err := parseJSONFile[JSONManifest](metaPath)
if err != nil {
return nil, err
}

return meta, nil
}
Loading
Loading