Changes for esp-idf-sys cmake build
#14
Conversation
Add dependency `remove_dir_all` Note: On windows `std::fs::remove_dir_all` doesn't really work (rust-lang/rust#29497).
Publicly export the `which` crate Remove `log` dependency of `cmd*` macros.
|
Looks OK to me. If you want me to merge it, I'll merge it? cmake utils and "maybe better python" can be another iteration. |
|
Not really sure about this file contents' comparison, but I assume your plan is to use this for small stuff, like sdkconfig's and similar? |
Yup. An addition to that would be to copy an entire directory if its content is different (and then only copy the files that are different), but I was too lazy to implement that just yet.
I'd prefer to merge only after I can at least build the |
Add dependency `cmake` Add initial subset of cmake file API Add function to extract defined variables in cmake script
Fix kconfig json parse bug
N3xed
changed the title
esp-idf-sys cmake buildesp-idf-sys cmake build
|
Okay, this should do it for now. I've also updated my PR on the |
| pub fn from_scons_vars(scons_vars: &pio::project::SconsVariables) -> Result<Self> { | ||
| let clang_args = cli::NativeCommandArgs::new(&scons_vars.incflags) | ||
| .chain(cli::NativeCommandArgs::new( | ||
| scons_vars | ||
| .clangargs | ||
| .as_deref() | ||
| .unwrap_or_default(), | ||
| )) | ||
| .collect(); |
There was a problem hiding this comment.
I've not tested this yet. But I guess this would only cause problems if SconsVariables::incflags and clangargs contained unescaped doube-quotes (") which would be removed by NativeCommandArgs on windows. And on unix unescaped ', " or \.
There was a problem hiding this comment.
Overall: I think the code quality is very good, so I'm willing to merge as-is (and to give you commit privileges if you want) afterwards.
There are two comments that need addressing (see below), but that you can surely fix post-merge.
There are a few more in the esp-idf-sys native build patch that definitely should be addressed, but I'll put them there, and the esp-idf-sys PR is still a Draft, so having those issues is OK.
I've given you approve privileges so you can merge yourself before/after fixing these changes.
Please let me know if you would like to have commit privileges.
Add `git::Repository::apply_once` Add `git::Repository::is_applied` Publicly export `anyhow` (hidden from docs) because some macros need it. Fix `cmd_spawn` Add `cmd` variation that returns `Result<ExitStatus>`
Check the cmake version for support of a specific file-api object kind.
I would very much appreciate that. We'd have to coordinate development then somehow, wouldn't we?
Nope, github still says "Only those with write access to this repository can merge pull requests.". |
Indeed. For small bugfixes, I think no coordination necessary. For refactoring (big API breakages) as well as for introduction of new APIs, we should probably still file PRs, or at least write a short suggestion in the form of a new issue so that we can peer review.
I've send you the invitation now. Once you confirm, you should be able to get write access. |
Agreed. Ok, I'll squash merge this now, so that I can continue with solving the command-line length problem. |
Third batch for #8.
Steps
Refactor(moved to build script)mcudetection (decouple it from platformio)pullif out of dateMaybe better python utilitiesDetect python executabledirectory/files if contents differ (only files for now)std::fs::remove_dir_alldoesn't really work on windows.cmakeutilities (add them here, commit them to thecmakecrate, or both)cmakefile apicmaketoolchain fileCMAKE_C_FLAGS/CMAKE_CXX_FLAGS/CMAKE_ASM_FLAGSextractioncmakecrate sets thesebindgenbindgen::BuilderfromcmakemetadataLinkArgsBuilder,CInclArgs,CfgArgsfor cmake