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 Module #4675

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

Conversation

bashar-515
Copy link
Contributor

@bashar-515 bashar-515 commented Jan 3, 2025

Search for a registry module's meta.json file in the top level of the module and in the parent directory of its executable entry point.

Testing

Running the robot linked in the Jira ticket no longer outputs the WARN log listed. first_run.sh is successfully sourced and executed, and a dist.first_run_succeeded file is created.
Note: This has only been tested with a registry module, not a local module.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 3, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 3, 2025
@bashar-515
Copy link
Contributor Author

Right now, the meta.json file is being found, but the path to the set up script it provides is being used relative to the execution directory - as opposed to the top level directory - when testing :/

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 3, 2025
@@ -282,7 +282,13 @@ func (m *Module) FirstRun(
logger.Debug("no first run script specified, skipping first run")
return nil
}
relFirstRunPath, err := utils.SafeJoinDir(unpackedModDir, meta.FirstRun)
var firstRunTopLevelDir string
Copy link
Contributor Author

@bashar-515 bashar-515 Jan 3, 2025

Choose a reason for hiding this comment

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

Before, meta.FirstRun was used relative to the directory specified by unpackedModDir. In the case that the module has a type of registry, this would just be the parent directory of the module's entry point executable - not necessarily the top level of its directory. Here, I've changed it so that if moduleWorkingDirectory is not the empty string (which is equivalent to saying that the module's type is registry and the VIAM_ROOT_MODULE environment variable has been set), then the module's top level directory is used to find and execute the specified "first run" script.

// 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")
Copy link
Contributor Author

@bashar-515 bashar-515 Jan 3, 2025

Choose a reason for hiding this comment

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

Here to line 400 is just a refactor - no functional change besides the additional string return value.

return meta, "", err
}

if m.Type == ModuleTypeRegistry {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, this function would automatically return an error and no meta information when m.Type == ModuleTypeRegistry

@bashar-515 bashar-515 requested a review from cheukt January 3, 2025 22:38
@bashar-515 bashar-515 marked this pull request as ready for review January 3, 2025 22:42
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 6, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants