-
Notifications
You must be signed in to change notification settings - Fork 0
RDKEMW-3106 : Ensure modules path works in all environments #17
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
Conversation
Reason for change: Cleaned up the code Test Procedure: build should be successful. Risks: low Priority: P2
38e64c1
to
f890afe
Compare
Reason for change: Cleaned up the code Test Procedure: build should be successful. Risks: low Priority: P2
f890afe
to
5e51018
Compare
Changes are good and validated with custom build. |
include/JavaScriptContextBase.h
Outdated
@@ -55,6 +55,10 @@ class JavaScriptContextBase:public IJavaScriptContext, public JavaScriptKeyListe | |||
std::string getUrl(); | |||
virtual void onKeyPress(struct JavaScriptKeyDetails& details); | |||
virtual void onKeyRelease(struct JavaScriptKeyDetails& details); | |||
|
|||
//newly added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the "newly added" comments - here and in the other places.
include/JavaScriptContextBase.h
Outdated
@@ -55,6 +55,10 @@ class JavaScriptContextBase:public IJavaScriptContext, public JavaScriptKeyListe | |||
std::string getUrl(); | |||
virtual void onKeyPress(struct JavaScriptKeyDetails& details); | |||
virtual void onKeyRelease(struct JavaScriptKeyDetails& details); | |||
|
|||
//newly added | |||
static void setEnvVariable(const char* name, const char* value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this method.
include/JavaScriptContextBase.h
Outdated
|
||
//newly added | ||
static std::string sModulesPath; | ||
std::string getModulesPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be static method?
src/JavaScriptContextBase.cpp
Outdated
JavaScriptContextFeatures::JavaScriptContextFeatures(bool embedThunderJS, bool embedWebBridge, bool enableWebSockerServer, ModuleSettings& moduleSettings):mEmbedThunderJS(embedThunderJS), mEmbedWebBridge(embedWebBridge), mEnableWebSockerServer(enableWebSockerServer), mModuleSettings(moduleSettings) | ||
{ | ||
} | ||
|
||
JavaScriptContextBase::JavaScriptContextBase(JavaScriptContextFeatures& features, std::string url, IJavaScriptEngine* jsEngine): mApplicationUrl(url), mEngine(jsEngine), mEmbedThunderJS(features.mEmbedThunderJS), mEmbedWebBridge(features.mEmbedWebBridge), mEnableWebSockerServer(features.mEnableWebSockerServer), mModuleSettings(features.mModuleSettings) | ||
{ | ||
//newly added | ||
JavaScriptContextBase::setEnvVariable("JSRUNTIME_MODULES_PATH", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don;t want this method. Only we need to read this env variable JSRUNTIME_MODULES_PATH and should take priority than other modules path
src/JavaScriptContextBase.cpp
Outdated
if (mEmbedThunderJS) | ||
{ | ||
if (sThunderJSCode.empty()) | ||
{ | ||
sThunderJSCode = readFile("modules/thunderJS.js"); | ||
std::string ThunderJS= sModulesPath + "thunderJS.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep all append inside readFile method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kindly address comments. Also, keep PR heading with ticketnumber:<title>
2f4d144
to
0475bf3
Compare
Reason for change: Removed unwanted comments newly added Test Procedure: build should be successful. Risks: low Priority: P2
0475bf3
to
093e915
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some changes to address. I will explain in meeting
home = PWD; | ||
} | ||
sModulesPath = home; | ||
if(setenv("JSRUNTIME_MODULES_PATH",home.c_str(),1)==0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1, Perfrom getenv and use it as sModulesPath if environment is set
2,Else, set module path to pwd+/modules as written
3, Don;t want multiple if else
|
||
JavaScriptContextFeatures::JavaScriptContextFeatures(bool embedThunderJS, bool embedWebBridge, bool enableWebSockerServer, ModuleSettings& moduleSettings):mEmbedThunderJS(embedThunderJS), mEmbedWebBridge(embedWebBridge), mEnableWebSockerServer(enableWebSockerServer), mModuleSettings(moduleSettings) | ||
{ | ||
} | ||
|
||
JavaScriptContextBase::JavaScriptContextBase(JavaScriptContextFeatures& features, std::string url, IJavaScriptEngine* jsEngine): mApplicationUrl(url), mEngine(jsEngine), mEmbedThunderJS(features.mEmbedThunderJS), mEmbedWebBridge(features.mEmbedWebBridge), mEnableWebSockerServer(features.mEnableWebSockerServer), mModuleSettings(features.mModuleSettings) | ||
{ | ||
getModulesPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename as populateModulesPath
@@ -77,6 +82,22 @@ std::string JavaScriptContextBase::readFile(const char *file) | |||
std::ifstream src_file(file); | |||
std::stringstream src_script; | |||
src_script << src_file.rdbuf(); | |||
if(src_script.str().empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1, Get one argument to readFile isModule (by default true). Pass it false, when we are calling this api for running local file/application
2, This will avoid read to fail everytime for all modules fist time.
3, Also, if isModule is set, read the file after appending modulespath, else ready directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kindly look at comments
Reason for change: Cleaned up the code Test Procedure: build should be successful. Risks: low Priority: P2
Reason for change: Cleaned up the code Test Procedure: build should be successful. Risks: low Priority: P2
Reason for change: Removed unwanted comments newly added Test Procedure: build should be successful. Risks: low Priority: P2
Reason for change: Cleaned up the code Test Procedure: build should be successful. Risks: low Priority: P2
Fetches modules directory path in generic way