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

Chore: Change the settings so the project scanner may work on different setups #13

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

idillon-sfl
Copy link
Member

No description provided.

client/README.md Outdated Show resolved Hide resolved
client/package.json Outdated Show resolved Hide resolved
client/package.json Outdated Show resolved Hide resolved
client/package.json Outdated Show resolved Hide resolved
client/package.json Outdated Show resolved Hide resolved
server/src/BitBakeProjectScanner.ts Outdated Show resolved Hide resolved
server/src/BitBakeProjectScanner.ts Outdated Show resolved Hide resolved
} else {
logger.info('not using env script')
scriptFileBuffer.push(
`export PYTHONPATH=${this._pathToBitbakeFolder}/lib:$PYTHONPATH`,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required? I have not yet encountered cases were I needed to tweak the PATH to run bitbake. This is usually done by oe_init_build_env.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a fallback in case there is no environment script, so the extension still can attempt to work. Maybe it should just fail?

Copy link
Member

Choose a reason for hiding this comment

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

Yes there will always be an environment file for calling bitbake. It is the recommended workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. I'll make this work into this PR: #16

server/src/BitBakeProjectScanner.ts Outdated Show resolved Hide resolved
server/src/BitBakeProjectScanner.ts Outdated Show resolved Hide resolved
@idillon-sfl idillon-sfl force-pushed the Chore-change-configuration-settings branch from ef8cec9 to c745070 Compare October 4, 2023 17:28
@idillon-sfl idillon-sfl force-pushed the Chore-change-configuration-settings branch from c745070 to 1916897 Compare October 4, 2023 17:32
@idillon-sfl idillon-sfl force-pushed the Chore-change-configuration-settings branch from 1916897 to 12f0eb5 Compare October 6, 2023 19:42
return this.executeCommand(scriptFileName)
private executeBitBakeCommand (command: string): string {
const scriptContent: string = this.generateBitBakeCommand(command)
return this.executeCommand(scriptContent)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see code checking that the command did succeed. Is it done by the libary by throwing?

Copy link
Member Author

Choose a reason for hiding this comment

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

spawnSync won't throw an error, but return a "SpawnSyncReturns" object, which contains both stderr and stdout. BitBake syntax errors are included into "stderr" and are not error for us.

## Set BitBake's path
Some features require to know where your BitBake's folder is located. The extension will by default assume it is located at the root of the project in a folder named `bitbake`. If your BitBake folder is located somewhere else, set its path in the settings in order to have full features.
## Setup the extension
In order to work properly, the extension needs to know your Bitbake's location and build folder. It will make the following assumptions:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can be more specific than "work properly". The extension highlighting works without for instance. Maybe just "For all the features to be enabled,"

Copy link
Member

Choose a reason for hiding this comment

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

I'd also rephrase "it will make the following assumptions" -> "It will look for it at these default locations"

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets remove it it in PR #16

"default": "vscode-bitbake-build",
"description": "Use this setting to specify the build folder. Use other folder than for your regular 'build' to avoid collisions."
"default": "oe-init-build-env",
"description": "Set the path to the environment script to configure the BitBake project. If the file does not exist, the extension will attempt to configure the environment variables by itself."
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should attempt to configure the variables ourselves. Let's always ask for an env script as is the recommended Yocto workflow.

}
} catch (error: any) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should catch the error here because we don't know how to handle it. All callers will also have to check for errors. Error handling should be done at the calling level that can recover, or abort and display an error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is outdated, it does not exist anymore

@deribaucourt
Copy link
Member

I'll aprove this PR since those fixes are addressed in other PRs.

@idillon-sfl idillon-sfl merged commit fe89eb6 into staging Oct 10, 2023
@idillon-sfl idillon-sfl deleted the Chore-change-configuration-settings branch October 12, 2023 21:15
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.

2 participants