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

This enables project(... VERSION x.y.z.a ...) and DESCRIPTION etc.. usage (IDFGH-11310) #12461

Closed

Conversation

kohait00
Copy link
Contributor

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.

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

Messages
📖 You might consider squashing your 4 commits (simplifying branch history).

👋 Welcome kohait00, thank you for your first contribution to espressif/esp-idf project!

📘 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 expect

Espressif 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.

  1. An internal issue has been created for the PR, we assign it to the relevant engineer
  2. They review the PR and either approve it or ask you for changes or clarifications
  3. Once the Github PR is approved, we synchronize it into our internal git repository
  4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing
    • At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
  5. If the change is approved and passes the tests it is merged into the master branch
  6. On next sync from the internal git repository merged change will appear in this public Github repository

🔁 You can re-run automatic PR checks by retrying the DangerJS action

Generated by 🚫 dangerJS against 96e2a8a

Copy link
Member

@igrr igrr left a 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.

tools/cmake/project.cmake Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
tools/cmake/project.cmake Outdated Show resolved Hide resolved
@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 25, 2023
@github-actions github-actions bot changed the title This enables project(... VERSION x.y.z.a ...) and DESCRIPTION etc.. usage This enables project(... VERSION x.y.z.a ...) and DESCRIPTION etc.. usage (IDFGH-11310) Oct 25, 2023
good point, a little more info is always good

Co-authored-by: Ivan Grokhotkov <igrokhotkov@gmail.com>
@kohait00
Copy link
Contributor Author

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:
I provide review changes ASAP, including ducumentation and the test case ideas for VERSION, LANGUAGES etc
Then depending on the time frame that you guys have we can decide who would write the unit tests

@kohait00
Copy link
Contributor Author

kohait00 commented Oct 25, 2023

the promised test cases:

syntax reference
https://cmake.org/cmake/help/latest/command/project.html

usual simple case may not break. Nothing more than that was possible before

project(hello_world)

this should be possible but wont work, due to project.cmake internal LANGUAGES definition

project(hello_world CSharp C CXX ASM)

bit more sophisticated with version, probably the future default

project(hello_world VERSION 2.4.3)

even more complicated, order as in CMake documentation

project(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES C CXX ASM)

order reversed, also in the languages definitions

project(hello_world VERSION 2.4.3.6 LANGUAGES CXX C ASM DESCRIPTION "Cool New Project" )

to trigger the special case of C at the end

project(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CXX ASM C )

adding more to languages

project(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CSharp C CXX ASM )

duplicates

project(hello_world VERSION 2.4.3 DESCRIPTION "Cool New Project" LANGUAGES CSharp C CXX ASM C )

full blown, strict docu conform order

project(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 order

project(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 gone

project(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 )

@kohait00
Copy link
Contributor Author

I had to force push because I forgot to correct the 'unlikly' and fixed it via amend..

so that's the only change there..
I did an amend on f0d8b3f (which I have seen you already cleared) and cherry pick on the documentation commit.. sorry for the hassle

there are no other changes so far

@kohait00 kohait00 requested a review from igrr October 26, 2023 08:05
@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Nov 1, 2023
@sudeep-mohanty
Copy link
Collaborator

sudeep-mohanty commented Nov 23, 2023

@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 project() argument gradually with some unit-tests for sanity so that we do not introduce any regression or breaking changes. Therefore, I would incorporate the addition of the VERSION argument first and then investigate the others subsequently.

@sudeep-mohanty sudeep-mohanty added the PR-Sync-Rebase Pull request sync as rebase commit label Nov 23, 2023
@sudeep-mohanty
Copy link
Collaborator

sha=96e2a8a51c7c9613d0699e46e00c4ca160b5eace

@sudeep-mohanty sudeep-mohanty added PR-Sync-Rebase Pull request sync as rebase commit and removed PR-Sync-Rebase Pull request sync as rebase commit labels Nov 23, 2023
@kohait00
Copy link
Contributor Author

@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 project() argument gradually with some unit-tests for sanity so that we do not introduce any regression or breaking changes. Therefore, I would incorporate the addition of the VERSION argument first and then investigate the others subsequently.

I dont mind at all.. Do whatever is desired, since I currently can't provide additional testcases apart from what I tested locally.
If there is anything else I can do here to support, let me know..
Have fun.

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Selected for Development Issue is selected for development labels Dec 6, 2023
espressif-bot pushed a commit that referenced this pull request Dec 8, 2023
…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 <sudeep.mohanty@espressif.com>
@Alvin1Zhang
Copy link
Collaborator

Thanks for contribution again, changes have been merged with 9beda4c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Rebase Pull request sync as rebase commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants