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.

Sign up for GitHub

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

rust: Require native: true for proc-macro targets and allow linki… #11743

Closed
wants to merge 1 commit into from
+10 −2
Closed

rust: Require native: true for proc-macro targets and allow linki… #11743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions mesonbuild/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1408,7 +1408,7 @@ def link(self, target):
msg = f"Can't link non-PIC static library {t.name!r} into shared library {self.name!r}. "
msg += "Use the 'pic' option to static_library to build with PIC."
raise InvalidArguments(msg)
if self.for_machine is not t.for_machine:
if self.for_machine is not t.for_machine and (not t.uses_rust() or t.rust_crate_type != 'proc-macro'):
Copy link
Member

Choose a reason for hiding this comment

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

I think t can be a CustomTarget which does not implement uses_rust(). What about:

Suggested change
if self.for_machine is not t.for_machine and (not t.uses_rust() or t.rust_crate_type != 'proc-macro'):
if self.for_machine is not t.for_machine and (not isinstance(t, SharedLibrary) or t.rust_crate_type != 'proc-macro'):

msg = f'Tried to mix libraries for machines {self.for_machine} and {t.for_machine} in target {self.name!r}'
if self.environment.is_cross_build():
raise InvalidArguments(msg + ' This is not possible in a cross build.')
Expand All @@ -1432,7 +1432,7 @@ def link_whole(self, target):
msg = f"Can't link non-PIC static library {t.name!r} into shared library {self.name!r}. "
msg += "Use the 'pic' option to static_library to build with PIC."
raise InvalidArguments(msg)
if self.for_machine is not t.for_machine:
if self.for_machine is not t.for_machine and (not t.uses_rust() or t.rust_crate_type != 'proc-macro'):
msg = f'Tried to mix libraries for machines {self.for_machine} and {t.for_machine} in target {self.name!r}'
if self.environment.is_cross_build():
raise InvalidArguments(msg + ' This is not possible in a cross build.')
Expand Down Expand Up @@ -2328,6 +2328,13 @@ def process_kwargs(self, kwargs):
else:
raise InvalidArguments(f'Invalid rust_crate_type "{rust_crate_type}": must be a string.')
if rust_crate_type == 'proc-macro':
if 'native' not in kwargs:
kwargs['native'] = True
if not kwargs.get('native', False):
Copy link
Member

Choose a reason for hiding this comment

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

You set it to true above if it's missing, so the False cannot be used.

Suggested change
if not kwargs.get('native', False):
if not kwargs['native']:

if self.environment.is_cross_build():
raise InvalidArguments('Rust "proc-macro" crate type requires "native: true". This will fail in a cross-build.')
else:
mlog.warning('Rust "proc-macro" crate type requires "native: true". This will fail in a cross-build.')
Comment on lines +2334 to +2337
Copy link
Member

Choose a reason for hiding this comment

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

It does not require "native: true", that's the default in this case. I think it should always be an hard error, no need to downgrade to a warning if it's not a cross build. To happen, user must have explicitely wrote:

shared_library(..., rust_crate_type: 'proc-macros', native: false)

which is always wrong.

What about:

Suggested change
if self.environment.is_cross_build():
raise InvalidArguments('Rust "proc-macro" crate type requires "native: true". This will fail in a cross-build.')
else:
mlog.warning('Rust "proc-macro" crate type requires "native: true". This will fail in a cross-build.')
raise InvalidArguments('Rust "proc-macro" crate type does not allow "native: false".')

FeatureNew.single_use('Rust crate type "proc-macro"', '0.62.0', self.subproject)

def get_import_filename(self) -> T.Optional[str]:
Expand Down
1 change: 1 addition & 0 deletions test cases/rust/18 proc-macro/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pm = shared_library(
'proc_macro_examples',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A couple of lines above, why is this test disabled on non-Linux, how does it fail? Theoretically there shouldn't be anything special required.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sdroege some of the tests are disabled on certain platforms because toolchains might not be setup on those platforms (work needed) or tests can take too long. In this case, you are probably running into the second option. Azure CI takes an ungodly amount of time and will timeout frequently.

Copy link
Member

Choose a reason for hiding this comment

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

It could very well be historical reasons that have been fixed since then. Feel free to open a separate PR that unskip them and see how it goes on the CI.

'proc.rs',
rust_crate_type : 'proc-macro',
native: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would I enable this test on the CI for actually doing cross-compilation? I see that there are Linux/armhf and mingw64 cross tests in the CI. Should I add the corresponding rust toolchains there? How would I then enable this test to actually run?

Copy link
Member

Choose a reason for hiding this comment

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

See e.g. cross/linux-mingw-w64-64bit.txt

You just need to set up the rust compiler as a cross binary, and configure the corresponding .json file to change e.g. "tests": ["common", "cmake"], to specify which run_project_tests.py suites are now supported by that cross test configuration.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add native: true here, the whole point of this PR is to test it's the default.

Giving it a try at cross rust compilation in CI there: #11990.

)

main = executable(
Expand Down