-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
This enables project(... VERSION x.y.z.a ...) and DESCRIPTION etc.. usage (IDFGH-11310) #12461
Conversation
👋 Welcome kohait00, thank you for your first contribution to 📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project. Pull request review and merge process you can expectEspressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.
🔁 You can re-run automatic PR checks by retrying the DangerJS action |
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.
Thanks for the nice improvement @kohait00!
The code looks good to me overall, I have left a few cosmetic comments. My main concern is that the part about LANGUAGES
seems to be a piece of fairly complex string/list manipulation code. I think we need to have some test coverage for it to prevent regressions.
Do you think you can add a few test cases to validate the behavior of this code?
I see we have two test cases for the version related logic,
def test_get_version_from_git_describe(test_git_template_app: Path, idf_py: IdfPyFunc) -> None: |
and, somewhat badly named,
def test_kconfig_get_version_from_describe(idf_py: IdfPyFunc, test_app_copy: Path) -> None: |
Probably it makes sense to
- create a new test file, test_versions.py
- move the two existing tests there
- add one new test case about version from
project
call - add another new test case (there or in another file) to validate the logic related to
LANGUAGE
argument handling
If you don't have time to work on adding test cases, please let us know anyway.
good point, a little more info is always good Co-authored-by: Ivan Grokhotkov <[email protected]>
regarding the oetending of test cases that @igrr suggested, I could try but to be honest, I am a newby in python, barely read it.. It would take some time for sure.. But then again, I am also learning CMake now.. Probably easier for me would be to provide some project(xxx) string that can be used as a starting point.. My suggestion: |
the promised test cases: syntax reference usual simple case may not break. Nothing more than that was possible beforeproject(hello_world) this should be possible but wont work, due to project.cmake internal LANGUAGES definitionproject(hello_world CSharp C CXX ASM) bit more sophisticated with version, probably the future defaultproject(hello_world VERSION 2.4.3) even more complicated, order as in CMake documentationproject(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES C CXX ASM) order reversed, also in the languages definitionsproject(hello_world VERSION 2.4.3.6 LANGUAGES CXX C ASM DESCRIPTION "Cool New Project" ) to trigger the special case of C at the endproject(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CXX ASM C ) adding more to languagesproject(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CSharp C CXX ASM ) duplicatesproject(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CSharp C CXX ASM C ) full blown, strict docu conform orderproject(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CSharp C CXX ASM HOMEPAGE_URL "http://www.hello-world.com") full blown, arbitrary orderproject(hello_world DESCRIPTION "Cool New Project" VERSION 2.4.3.6 LANGUAGES CSharp C CXX ASM HOMEPAGE_URL "http://www.hello-world.com") this schows that not all cases are caught, description will result in 'Cool new Project', the C at the end is goneproject(hello_world VERSION 2.4.3.6 LANGUAGES CXX C ASM DESCRIPTION "Cool New ProjectC" ) this wont ever work,project(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" CSharp C CXX ASM C ) |
ffe23aa
to
96e2a8a
Compare
I had to force push because I forgot to correct the 'unlikly' and fixed it via amend.. so that's the only change there.. there are no other changes so far |
@kohait00 Thanks for the suggestions and apologies for getting back to you late on this. I would be pulling in your changes but we might have to enable each |
sha=96e2a8a51c7c9613d0699e46e00c4ca160b5eace |
I dont mind at all.. Do whatever is desired, since I currently can't provide additional testcases apart from what I tested locally. |
…make This commit enables the standad VERSION argument for the project() macro in ESP-IDF. The VERSION argument is compilant with the requirements of cmake 3.16. This commit also adds new test cases for verifying the validity of the version argument. Merges #12461 Co-authored-by: Sudeep Mohanty <[email protected]>
Thanks for contribution again, changes have been merged with 9beda4c. |
the current implementation of project.cmake
overrides the
project()
function with a macro which then calls the overridden function, but only provides the project name along with some predefined LANGUAGES..
in case someone wanted to specify additional LANGUAGES, DESCRIPTION etc... this is entirely ignored.
probably to keep things simple.
this fix enables the usage of all the parameters for project() including LANGUAGES
it splits the provided ARGV when there is LANGUAGES present, and eliminates the predefined languages that the project.cmake already wants to specify itself.
everything else is untouched.
this also has the benefit that the cmake own PROJECT_VERSION now is able to be specified via the project(VERSION x.y.z.a ...) and then can be used to initalize the PROJECT_VER, no need to specify the PROJECT_VER explicitly as a set(PROJECT_VER x.y.z.a)..
I admit that the solution is probly not optimal, feel free to improve on it.