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

Fix #13043 (Added Support for Shared Items Projects) #6286

Merged
merged 34 commits into from
Aug 27, 2024

Conversation

DevFelixFaber
Copy link
Contributor

@DevFelixFaber DevFelixFaber commented Apr 12, 2024

At work (Areus Gmbh), we use Shared Items Projects in order to deduplicate code. In contrast to conventional project files (.vcxproj), the compilation configuration is set by the "includee", which makes the code-reuse of the project more flexible.

This pull-request adds support for Shared Items Projects (.vcxitems) in Visual studio projects / solutions.

The idea is that for each .vcxproj, we check for referenced shared items projects. When all the source files and include paths are build, we add the ones defined in the referenced .vcxitems.
The implementation probably isn't 100% robust, but this works for our projects and could serve as a starting point :)

I apologize for the commit history - it's a bit wonky as we currently still use 2.11. I tried to merge the changes to the main branch so other could benefit from these changes as well, but seems like the git history looks a bit weird as a result.

lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
@firewave
Copy link
Collaborator

Thanks for a contribution. I will take a closer look in the next few days.

Please add a test with an example project to test/cli/more-projects.py.

Also some initial comments inline.

@firewave
Copy link
Collaborator

I apologize for the commit history - it's a bit wonky as we currently still use 2.11. I tried to merge the changes to the main branch so other could benefit from these changes as well, but seems like the git history looks a bit weird as a result.

That can be resolved by re-basing the branch on the latest head.

@DevFelixFaber
Copy link
Contributor Author

Thank you for the feedback, I'll try to implement it this week.

@DevFelixFaber
Copy link
Contributor Author

DevFelixFaber commented Apr 19, 2024

I apologize for the commit history - it's a bit wonky as we currently still use 2.11. I tried to merge the changes to the main branch so other could benefit from these changes as well, but seems like the git history looks a bit weird as a result.

That can be resolved by re-basing the branch on the latest head.

I had the same issue as my merge, but my oversight was that my initial changes came from the 2.11 release branch, meaning it includes the few commits made for the official release.
I re-based this once more, removed the release commits and force-pushed this to my branch. Seems more correct now.

Still working on adding tests.

@firewave
Copy link
Collaborator

Thanks for adding the project. This will help understanding things. It is probably sufficient to have only a single configuration in it. That should make things easier to read as well.

@DevFelixFaber
Copy link
Contributor Author

DevFelixFaber commented Apr 30, 2024

Thanks for adding the project. This will help understanding things. It is probably sufficient to have only a single configuration in it. That should make things easier to read as well.

Fixed in 10fb4e5

I also added a (crude) test in d34d14c. Seems to run fine on my machine, but I'm not sure I set it up correctly.

I seem to have broken the "changes requested" with my rebase + force push. Sorry!

@firewave
Copy link
Collaborator

Sorry for the late reply.

Please fix the CI and I will finally have a proper look.

@DevFelixFaber DevFelixFaber marked this pull request as draft June 5, 2024 10:57
@DevFelixFaber DevFelixFaber marked this pull request as ready for review June 5, 2024 10:57
@DevFelixFaber
Copy link
Contributor Author

My apologies, I borked the pipeline with the code removed in a00dd2a.
That was some special code which (should be) unrelated to the shared items support.
I fixed some more formatting issues (thanks Visual Studio) and adjusted the assertion on the new shared items project test.

@firewave
Copy link
Collaborator

firewave commented Jun 5, 2024

My apologies, I borked the pipeline with the code removed in [a00dd2a]

It happens.

As the CI seems to be happy now, I hope to finish up the review this week.

@danmar
Copy link
Owner

danmar commented Jun 9, 2024

At a glance this looks very interesting. Please make the CI happy. 👍

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

nits

lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
@DevFelixFaber
Copy link
Contributor Author

DevFelixFaber commented Aug 9, 2024

@firewave any updates on this? I fixed the merge conflicts that were introduced over time. Pipeline is happy-ish (pipeline used to work fine. Now it's failing a test unrelated to my changes, so I'm not sure how to address this)

@firewave
Copy link
Collaborator

firewave commented Aug 9, 2024

any updates on this?

Sorry about that. I keep running into existing (long-standing) issues to look into (which mostly block my own in-progress stuff) and forgetting looking into other peoples stuff.

I fixed the merge conflicts that were introduced over time. Pipeline is happy-ish (pipeline used to work fine. Now it's failing a test unrelated to my changes, so I'm not sure this is my fault)

It is all fine. That is a known flake.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

more nits

lib/importproject.cpp Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.h Outdated Show resolved Hide resolved
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

some more nits.

lib/importproject.h Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
lib/importproject.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

lgtm

@danmar
Copy link
Owner

danmar commented Aug 21, 2024

@DevFelixFaber If nobody has complaints in 2-3 days I think we can merge this. Feel free to ping me..

@DevFelixFaber
Copy link
Contributor Author

Seems like there are no complaints! @danmar pinging as requested.

Thanks for the review(s) :)

@danmar danmar merged commit af5b6ef into danmar:main Aug 27, 2024
63 checks passed
@danmar danmar changed the title Added Support for Shared Items Projects Fix #13043 (Added Support for Shared Items Projects) Aug 27, 2024
@danmar
Copy link
Owner

danmar commented Aug 27, 2024

Trac ticket: https://trac.cppcheck.net/ticket/13043

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants