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.

Sign up for GitHub

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

Jump to bottom

pkg/nimble: Replace double quotes with single quotes for two CFLAGS #18376

Merged
merged 1 commit into from
Aug 4, 2022
Merged

pkg/nimble: Replace double quotes with single quotes for two CFLAGS #18376

merged 1 commit into from
Aug 4, 2022

Conversation

blueted2
Copy link
Contributor

Contribution description

Using double quotes can cause issues when the final cmake command is called, and you end up with un-escaped nested double quotes.

Testing procedure

Issues/PRs references

@blueted2
pkg/nimble: Replace double quotes with single quotes for two CFLAGS
25a476f 
Using double quotes can cause issue when the final cmake command is called, and you end up with un-escpaed nested double quotes.
@github-actions github-actions bot added Area: BLE Area: Bluetooth Low Energy support Area: pkg Area: External package ports labels Jul 27, 2022
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 3, 2022
maribu
maribu approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Our CI should be able to detect if this causes any regression. If not and this fixes your use case, let's have it in.

@maribu maribu enabled auto-merge August 3, 2022 08:08
@blueted2
Copy link
Contributor Author

blueted2 commented Aug 3, 2022

Thanks !
Looking back at the command, most of the the other double quotes had been correctly escaped, it was just for those two arguments I had issues.

The bit that gave me issues is this:
... -DMYNEWT_VAL_MSYS_1_BLOCK_SIZE="(MYNEWT_VAL_BLE_L2CAP_COC_MPS + 48)" ...

I have included the final Cmake command: cmake_command.txt

@maribu
Copy link
Member

maribu commented Aug 4, 2022

The two CI failures are unrelated glitches

@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 4, 2022
@maribu maribu merged commit 3c07086 into RIOT-OS:master Aug 4, 2022
@blueted2 blueted2 deleted the single-quotes branch August 11, 2022 11:42
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

Assignees
No one assigned
Labels
Area: BLE Area: Bluetooth Low Energy support Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
2 participants
@blueted2 @maribu