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

Set the rpath to Qt for Rust binaries #797

Closed
wants to merge 1 commit into from

Conversation

tronical
Copy link
Member

@tronical tronical commented Jan 4, 2022

For C++ apps we apply rpath to the cdylib implicitly via qttypes, but
link flags for rust binaries are not transitively propagated but require
explicit opt-in by the top-level crate. However we want that
convenience, to avoid Rust apps having to deal with Qt backend specific
code in their build.rs. Therefore we use the _DEP mechanism to get the
qt library path from the qttypes crate and write it into a generic file
in the qt backend's build.rs, which is then picked up by sixtyfps-build.

We'll use the same mechanism to propagate link flags for the MCU build
(such as the linker script).

cc #566

@tronical
Copy link
Member Author

tronical commented Jan 4, 2022

What do you think? Is this too controversial? :)

@ogoffart
Copy link
Member

ogoffart commented Jan 4, 2022

I don't really like this hack and i whish there was better way.
The main problem with this in this state is that the file is not removed if we change the feature set.
Related of course if the fact that two backend being build will conflict on this file location
I really think that something needs to be done in rust so we can forward this kind of flag.

Then there is also the question whether the rpath should be set at all. This will break when shipping apps, wouldn't it?

@tronical
Copy link
Member Author

tronical commented Jan 4, 2022

I don't really like this hack and i whish there was better way.

Me, too - FWIW.

I really think that something needs to be done in rust so we can forward this kind of flag.

I wish so, too - but you might also remember the discussion we had upstream...

Related of course if the fact that two backend being build will conflict on this file location

Yes. Another option would be to create a different file per-backend, but I don't know how to determine which backends are actually linked in.

Then there is also the question whether the rpath should be set at all. This will break when shipping apps, wouldn't it?

Yes, that's a valid question and I think it's a bit orthogonal since it applies to the qttypes crate as well. The create also unconditionally sets rpath for cdylib target crates.

The current cargo behavior is inconsistent IMO:

  1. Library linkage is propagated (otherwise it would be impossible to use a -sys crate deep down in the hierarchy)
  2. Link flags are propagated if the final target crate is a cdylib.
  3. Otherwise link flags are not propagated.

Maybe (2) will be removed in the future, but that still leaves me thinking that (1) and (3) are not consistent with each other.

I think that we can either try to work around this inconsistency or - alternatively - we should drop all special handling. That includes qttypes emitting cdylib-link-args with rpath. Users of SixtyFPS, regardless of C++ or Rust, will always have to set LD_LIBRARY_PATH/DYLD_FRAMEWORK_PATH, regardless of whether they are developing or whether users are running a distributed binary.

Instead we could try to invest time into enhancing tooling that facilitates creating bundles with relative rpaths on macOS (we don't have a deployment story there at all right now) and for Linux either adding relative rpaths or making it easy to create snaps/flatpacks.

I was thinking that this approach would make life easier for existing users (since it's been reported before as an issue), especially in the light of us needing such a mechanism (propagating linker flags) for the mcu build.

@ogoffart
Copy link
Member

ogoffart commented Jan 4, 2022

it applies to the qttypes crate as well.

Right, in fact, the qttypes create is the crate that should somehow set the rpath. But unfortunately that's impossible :-(

@hunger
Copy link
Member

hunger commented Jan 5, 2022

I am probably missing something since I never tried this in practice, but this is how I understood this to work in theory:

A crate can export arbitrary metadata by just printing KEY=VALUE from its build script. Those values are available anywhere that library is linked to as DEP_<depname>_KEY in the environment of the crate that depends on the one exporting extra metadata.

What am I missing?

@tronical
Copy link
Member Author

tronical commented Jan 5, 2022

I am probably missing something since I never tried this in practice, but this is how I understood this to work in theory:

A crate can export arbitrary metadata by just printing KEY=VALUE from its build script. Those values are available anywhere that library is linked to as DEP_<depname>_KEY in the environment of the crate that depends on the one exporting extra metadata.

What am I missing?

I think your observation is correct, this is also how I understand the meta-data handling works.

@ogoffart
Copy link
Member

ogoffart commented Jan 5, 2022

What am I missing?

The DEP_<depname>_KEY are only available for direct dependency.

So what we have is a graph like this:

               your_crate
   /                                 \
sixtyfps_build                      sixtyfps 
 |                                        \
 sixtyfps_compiler           sixtyfps_backend_default
                                              |
                                     sixtyfps_backend_qt
                                              |
                                          qttypes

as you see, sixtyfps_backend_qt is not a direct dependency of sixtyfps_build. Not even a dependency at all.
your_crate is also not a direct dependent of sixtyfps_backend_qt.

Ideally, since it is the qttypes crate that is the first to against Qt, it should be that crates that sets the linking flags.

@hunger
Copy link
Member

hunger commented Jan 5, 2022

So why do you need that file?

qttypes has a build.rs script, so it can export the QT_RPATH.

Our qt backend has another build.rs file, so that can read DEP_qttypes_QT_RPATH and set BACKEND_RPATH.

rendering-backend-default also fas a build.rs, so it can collect all the DEP_rendering-backend-foo_RPATH and calculate the combined RPATH.

The sixtyfps crate depends on rendering-backend-default, so it can get and set that combined rpath via linker flags.

User programs directly depend on sixtyfps, shouldn't they just do the right thing if all the forwarding was implemented?

@tronical
Copy link
Member Author

tronical commented Jan 5, 2022

User programs directly depend on sixtyfps, shouldn't they just do the right thing if all the forwarding was implemented?

How do user programs get the rpath into their rust link flags?

@hunger
Copy link
Member

hunger commented Jan 5, 2022

I thought that -sys crates did the right thing wrt. linker flags, and sixtyfps would essentially be a pretend--sys crate after all this passing of flags. Seems like that is the part I had been missing:-)

sixtyfps-build is of course a different story. But I thought that only needed the rpath flags to pass those up to the user crate?

@ogoffart
Copy link
Member

ogoffart commented Jan 5, 2022

I thought that -sys crates did the right thing wrt. linker flags

That's the problem: they don't. See the linked rust issue from the comment and the original issue.

@hunger
Copy link
Member

hunger commented Jan 5, 2022

Ahhhh!

I understood that the flags do not get passed on, but I had had read the issue to be limited to transitiv dependencies and had somehow assumed direct ones would be fine for some reason.

Now this patch actually makes sense:-)

@hunger
Copy link
Member

hunger commented Jan 5, 2022

Could you propagate the link flags up to the default rendering backend and have that export the file though?

We would have one place to aggregate all the different linker flags then.

@tronical
Copy link
Member Author

tronical commented Jan 5, 2022

Could you propagate the link flags up to the default rendering backend and have that export the file though?

We would have one place to aggregate all the different linker flags then.

I think that's a good idea. I discarded it earlier because the default backend wouldn't depend on the mcu backend (because we won't publish it for now), but that could be dealt with in a patch on top in the mcu branch.

@tronical tronical force-pushed the simon/qt-rpath-propagation branch 2 times, most recently from 0776518 to f6e7c12 Compare January 5, 2022 15:07
For C++ apps we apply rpath to the cdylib implicitly via qttypes, but
link flags for rust binaries are not transitively propagated but require
explicit opt-in by the top-level crate. However we want that
convenience, to avoid Rust apps having to deal with Qt backend specific
code in their build.rs. Therefore we use the _DEP mechanism to get the
qt library path from the qttypes crate, forward it to the default
backend crate and write it into a generic file in the qt backend's
build.rs, which is then picked up by sixtyfps-build.

In the future, we'll use the same mechanism to propagate link flags and
extend the linker serach path for the MCU build (such as the linker
script).

cc #566
@ogoffart
Copy link
Member

ogoffart commented Jan 5, 2022

Another reason why not do this is that this doesn't work with the sixtyfps! macro

@tronical
Copy link
Member Author

tronical commented Jan 5, 2022

Ok, so we're back to feature flags in the public sixtyfps-build crate for the MCU then :( (which also doesn't work with the macro, but that's a different topic)

Shall I make changes to qttypes to remove the rpath emission there?

@tronical tronical closed this Jan 5, 2022
@tronical tronical deleted the simon/qt-rpath-propagation branch January 5, 2022 15:31
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