ccollins476ad commented on issue #88: [RFC] Add option to generate CMakeLists.txt file for a target URL: https://github.com/apache/mynewt-newt/pull/88#issuecomment-322291173 Overall, it looks good to me. However, I have a few comments. A lot of these are pretty minor, so you can use your own discretion when deciding whether to make any changes. 1. Writing `CMakeLists.txt` to the project directory concerns me just a little bit, for a few reasons: a. It "pollutes" the project directory with a generated file. Aside from this file, newt writes all its files to the `bin` directory. b. It limits the user to one target at a time. Other commands, e.g., `build`, allow multiple target names to be specified for a single invocation. When automating builds, it is useful to be able to specify multiple targets or `all`. I am not sure how people would end up using cmake with newt, so the above concern may be invalid. If this feature is intended for manual use only, then the current implementation might be perfect. 2. The help text for `target cmake` looks a bit off: `Cmake all the variables for the target specified by <target-name>.` 3. The `CMakeTargetWrite()` and `CMakeBuildPackageWrite()` functions should use `StatusMessage()` to display text rather than `fmt.Printf()`. That way, the user can silence the output if desired. I would use a verbosity of `util.VERBOSITY_DEFAULT` here. 4. The above two functions print `Generating code for <package>`. That seems slightly inaccurate since no code is generated. I'm afraid I don't have any suggestions here :). 5. `cmake.go` redefines the compiler types. Would it be better to just use the original definitions in the `toolchain` package (e.g., `toolchain.COMPILER_TYPE_C`)? 6. In `CMakeTargetGenerate()`: ``` CmakeFileHandle, _ := os.Create(CmakeListsPath()) ``` It seems dangerous to ignore the error here. I suggest checking the error and returning `ChildNewtError(err)` on failure. It is important to wrap the original error with `ChildNewtError()` in case the calling code needs to know the specific file system error that caused the problem. 7. In `CMakeTargetGenerate()`: ``` targetBuilder, err := NewTargetBuilder(target) if err != nil { return util.NewNewtError(err.Error()) } ``` `NewTargetBuilder()` already returns a NewtError, so this code can just return `err` unadorned. By returning the original error, you preserve the stack trace. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
With regards, Apache Git Services
