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

ADD linkbefore and linkafter option #250

Merged
merged 8 commits into from
Aug 28, 2018
Merged

ADD linkbefore and linkafter option #250

merged 8 commits into from
Aug 28, 2018

Conversation

louxiu
Copy link
Contributor

@louxiu louxiu commented Aug 12, 2018

To have chance to add more linker options. #247

@saudet
Copy link
Member

saudet commented Aug 17, 2018

Allowing to set compiler options directly in the Java files makes it harder to set properties for multiple platforms. Also, it's just occurred to me that we can probably already use platform.link.prefix and platform.link.suffix to solve this issue cleaning, for example:

platform.link.prefix=-Wl,--whole-archive -l
platform.link.suffix= -Wl,--no-whole-archive

Should work if we update the Builder a bit to split the properties into multiple options at the white spaces, like it's already doing for platform.compiler. Could you give that a try?

@louxiu
Copy link
Contributor Author

louxiu commented Aug 19, 2018

Sorry for late reply. It's a clever way. 😃 I resubmit a commit. And should we change the property file(linux-x86_64.properties etc) or there is another way to override the default value of platform.link.prefix and platform.link.suffix?

@saudet
Copy link
Member

saudet commented Aug 19, 2018

Yes, that's what the "properties" in the command line option and in the Mojo configuration are for. I wonder if we can make this the default though?

We want to merge the last prefix with the library name, but only when it doesn't end with a space, and similarly for the first suffix. Are you sure your changes do that?

@louxiu
Copy link
Contributor Author

louxiu commented Aug 19, 2018

Ok, I know what's your preferred way. Please check it again.

@saudet
Copy link
Member

saudet commented Aug 22, 2018

No, that doesn't split all spaces. We can do like it was before, but consider the parts after the first "word" in the suffix as independent options, and similar for prefix.

@louxiu
Copy link
Contributor Author

louxiu commented Aug 24, 2018

@saudet done

@saudet
Copy link
Member

saudet commented Aug 27, 2018

Looking better, almost there, but with the following properties when there are no libraries to link (like with the unit tests that you can try by running mvn test) it appends a -Wl,--whole-archive without a -Wl,--no-whole-archive. Moreover, when we do specify a library to link, the suffix gets appended to the library name like -lfoo-Wl,--no-whole-archive, when that should be 2 separate options.

platform.link.prefix=-Wl,--whole-archive -l
platform.link.suffix= -Wl,--no-whole-archive

@louxiu
Copy link
Contributor Author

louxiu commented Aug 27, 2018

@saudet I know the problem. Space before link suffix in properties should be \u0020
platform.link.prefix=-Wl,--whole-archive -l
platform.link.suffix=\u0020-Wl,--no-whole-archive

And I modified the code, ignore link before and after options if there is no lib links

@saudet
Copy link
Member

saudet commented Aug 27, 2018

Ah, yes, good catch! That makes it a bit harder to use, but oh well. Using a backslash to escape like this also works:

platform.link.prefix=-Wl,--whole-archive -l
platform.link.suffix=\ -Wl,--no-whole-archive

Anyway, now it looks like the whole prefix gets appended to the library name in only 1 option, including the space, like this: g++ ... '-Wl,--whole-archive -lfoo' .... Those need to be 2 options.

@louxiu
Copy link
Contributor Author

louxiu commented Aug 27, 2018

No, if I test correctly. The latest commit has splitted -Wl,--whole-archive -l as 2 options.

@saudet
Copy link
Member

saudet commented Aug 27, 2018

You're right, I was testing on the wrong branch, sorry. Seems to work correctly, but it's stripping the options in between each library. I'm not sure it always makes sense to do that? It wouldn't hurt to leave them at least... What do you think?

@louxiu
Copy link
Contributor Author

louxiu commented Aug 27, 2018

I think I got what you mean finally.
It should be

-Wl,--xxx -Wl,--whole-archive -lfoo -Wl,--no-whole-archive -Wl,--whole-archive -lbar -Wl,--no-whole-archive 👍
-Wl,--xxx -Wl,--whole-archive -lfoo -lbar -Wl,--no-whole-archive 👎
'-Wl,--xxx -Wl,--whole-archive' -lfoo -Wl,--no-whole-archive '-Wl,--xxx -Wl,--whole-archive' -lbar -Wl,--no-whole-archive 👎

Sorry for my bad English. Is it right?

@saudet
Copy link
Member

saudet commented Aug 27, 2018

I think that would be best, yes, but do you think that's OK?

@saudet
Copy link
Member

saudet commented Aug 27, 2018

It would actually be like this though:

-Wl,--xxx -Wl,--whole-archive -lfoo -Wl,--no-whole-archive -Wl,--xxx -Wl,--whole-archive -lbar -Wl,--no-whole-archive

@louxiu
Copy link
Contributor Author

louxiu commented Aug 27, 2018

Yes, I prefer that way too. It keeps meaning of link.prefix and link.suffix. I misunderstand your comment. 😢

We want to merge the last prefix with the library name, but only when it doesn't end with a space, and similarly for the first suffix.

@saudet saudet merged commit d8a0f91 into bytedeco:master Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants