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

multiple definition of crow::method_strings #350

Closed
nekoffski opened this issue Feb 21, 2022 · 6 comments · Fixed by #354
Closed

multiple definition of crow::method_strings #350

nekoffski opened this issue Feb 21, 2022 · 6 comments · Fixed by #354
Labels
bug Something isn't working

Comments

@nekoffski
Copy link

Hi,

I had just updated the crow submodule for my project and I faced the below linker error:

/usr/bin/ld: CMakeFiles/sparkle-connector.dir/api/BoardController.cpp.o:(.data.rel.local+0x0): multiple definition of `crow::method_strings'; CMakeFiles/sparkle-connector.dir/api/ApiService.cpp.o:(.data.rel.local+0x0): first defined here
/usr/bin/ld: CMakeFiles/sparkle-connector.dir/main.cpp.o:(.data.rel.local+0x0): multiple definition of `crow::method_strings'; CMakeFiles/sparkle-connector.dir/api/ApiService.cpp.o:(.data.rel.local+0x0): first defined here
collect2: error: ld returned 1 exit status

The cause of the issue is based here:

const char* method_strings[] = ...

Including the above header in at least 2 different compilation units that will be linked together ends up generating 2 same symbols for this variable.

I'm a bit confused because GH actions show that all checks on the master branch pass. Did I do something wrong (e.g. crow is not expected to be included in more than a single file, but that would be at least strange) or there is something missing in your test harness?

If it is a bug, I recommend changing method_strings to a static local variable (it seems that it is used only in a single function but I haven't investigated it deeply).

Thanks in advance for the clarification :)

@The-EDev The-EDev added the bug Something isn't working label Feb 21, 2022
@The-EDev The-EDev changed the title Possible bug - multiple definition of `crow::method_strings' Possible bug - multiple definition of crow::method_strings Feb 21, 2022
@The-EDev The-EDev changed the title Possible bug - multiple definition of crow::method_strings multiple definition of crow::method_strings Feb 21, 2022
@The-EDev
Copy link
Member

Hello @nekoffski and thanks a lot for pointing this bug out.

This was introduced in #332, I should've probably looked at #280 and used constexpr const or static. I'll work on fixing it ASAP.

To address your confusion, we've had multiple ways to test building with multiple source files (something that Crow should absolutely support without issue), however to date, none have been standardized or put into the CI process. I believe @luca-schlecker has made an example while working on #280. Though I don't recall if I ever managed to get it working on my own machine.

Issues like this and #347 are expected these weeks because of how large #332 is, no matter how well the testing is, issues were bound to occur and I'm glad people are pointing them out so soon.

While you're not obligated to, it would be great if you could provide a basic version of your code which is producing this error so it can be integrated into tests (this is just to save some time, please only do it if it's convenient for you).

Thanks again for pointing this issue out, your help is very much appreciated!

@nekoffski
Copy link
Author

Cool, thanks for the explanation!

Of course, I would be delighted to help you at least by providing some examples. Should I create a separated branch and make a subfolder in examples or do you prefer me to do this in another way?

@The-EDev
Copy link
Member

Should I create a separated branch and make a subfolder in examples or do you prefer me to do this in another way?

Whichever way is most convenient for you is fine. Whether you want to open a PR with a new example subfolder or simply post some code in a comment here, both are awesome!

@nekoffski
Copy link
Author

As soon as I find some free time I will simplify the code to get rid of other 3rdparty dependencies and open a PR. Thanks.

@nekoffski
Copy link
Author

I couldn't create a PR directly to the repository because of the permissions so I've forked it, here is the commit. It compiles only with the bug fixed. Feel free to reuse it if you want :)

@The-EDev
Copy link
Member

Thanks a lot, I'll put it to good use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants