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

[WIP] Add Cargo buildsystem #564

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chriskilding
Copy link

Adds a Flatpak buildsystem for Cargo.

Implements #15


Note: I am opening this PR at an early stage to gather initial feedback and requirements for what needs to be done. I'm not familiar with the internals of Flatpak, so will gratefully accept any help.

@chriskilding
Copy link
Author

I'm also a bit stuck on how to add the new test cases to the script / entry point that runs the test suite - could someone advise where this is done, so that they can be run in the build?

@TingPing
Copy link
Member

The entry point is https://github.com/flatpak/flatpak-builder/blob/main/tests/test-builder.sh

They are just simple shell scripts.

@TingPing
Copy link
Member

So we need to make this have value beyond what buidlsystem: simple just calling cargo does. That means that we have something that actually expands Cargo sources or similar.

@chriskilding
Copy link
Author

chriskilding commented Oct 31, 2023

@TingPing agreed - that was always the intention. I just wanted to get the test harness set up around it first to TDD my changes.

Would you still recommend tomlc99, or are there any better (perhaps Cargo-specific) parser options? Ideally we'd want a C TOML library which is readily installable from the OS package manager.

@TingPing
Copy link
Member

TingPing commented Oct 31, 2023

I have no opinion at all of TOML libraries.

On Fedora these are packaged:

@TingPing
Copy link
Member

Not packaged but maybe interesting:

@TingPing
Copy link
Member

I think I'd lean towards tomlc99 as a Meson subproject.

@chriskilding
Copy link
Author

Another thought about parsing the Cargo / Cargo.lock files....

If we take the Toml parser route with tomlc99 or similar, effectively we would have to reimplement all of Cargo's parse+fetch logic in C. Furthermore the Cargo project occasionally changes the Cargo.lock file format (as indicated by its version number), so each time this changes we'd have to update the flatpak-builder reimplementation. This will likely be non-trivial.

I am therefore thinking that either of the following strategies would be better than reimplementing the Cargo dependency fetch logic ourselves:

  1. Use the cargo crate and some kind of Rust-C FFI bindings to call the fetch Rust function from C directly. (This presumes that this function exists and is publicly available within Cargo's internal library. I imagine the FFI bindings would make this option more difficult to implement.)
  2. Do a shell call within flatpak-builder to effectively exec cargo fetch. (I think this will be simpler to implement, and also make it easier to accommodate newer versions of Cargo and the Cargo.lock file format. We would however have to take care to show any errors from the delegated cargo fetch call to the user.)

Thoughts?

@@ -1585,6 +1587,8 @@ builder_module_build_helper (BuilderModule *self,
simple = TRUE;
else if (!strcmp (self->buildsystem, "qmake"))
qmake = TRUE;
else if (!strcmp (self->buildsystem, "cargo"))
cargo = TRUE;
Copy link
Contributor

Choose a reason for hiding this comment

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

this will need to be documented. See data/flatpak-manifest.schema.json and doc/flatpak-manifest.xml

@@ -1741,6 +1761,10 @@ builder_module_build_helper (BuilderModule *self,
configure_cmd = "meson";
configure_final_arg = g_strdup ("..");
}
else if (cargo)
{
// nothing to do - Cargo does not have a configure step
Copy link
Contributor

Choose a reason for hiding this comment

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

so how do you select features?
or building a specific binary.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Just to be more specific about this, do you mean switches like:

  • Profile: cargo build --release / --debug
  • Target: cargo build --target <architecture>
  • Cargo features: cargo build --features <feature>,<feature>,...

@hfiguiere
Copy link
Contributor

Also one key thing here is that cargo isn't part of the base sdk. So the failure mode might not be ideal.

@chriskilding
Copy link
Author

@hfiguiere yes, also a good point. I suppose this prompts the question of how flatpak-builder should provide buildsystems for any language/toolchain that's not in the base SDK - not just Rust/Cargo.

@chriskilding
Copy link
Author

chriskilding commented Nov 21, 2023

One possibility is that the buildsystem API could provide a feature where you can declare the sdk-extensions that the buildsystem depends on.

There would then be a couple of options for how to resolve those sdk-extensions when needed. A 'soft' resolution option would be to use the extension if it's installed, but if it's not, flatpak-builder prompts the user to add them to the Flatpak manifest with a message like:

"The buildsystem depends on the following sdk-extensions [ foo, bar, baz ], but these were not present. Please declare them in your Flatpak manifest."

Meanwhile a more automated option would be for flatpak-builder to just go ahead and resolve all the buildsystem's sdk-extension dependencies by itself. This would, however, mean that those sdk-extensions are not declared in the Flatpak manifest. There might be unintended consequences to that, but I don't have a view yet on what those could be.

@alufers
Copy link

alufers commented Jan 8, 2024

"The buildsystem depends on the following sdk-extensions [ foo, bar, baz ], but these were not present. Please declare them in your Flatpak manifest."

Meanwhile a more automated option would be for flatpak-builder to just go ahead and resolve all the buildsystem's sdk-extension dependencies by itself. This would, however, mean that those sdk-extensions are not declared in the Flatpak manifest. There might be unintended consequences to that, but I don't have a view yet on what those could be.

The problem I see with this is that you can choose multiple different rust SDK extensions (stable, nightly etc.):

https://github.com/flathub/org.freedesktop.Sdk.Extension.rust-stable
https://github.com/flathub/org.freedesktop.Sdk.Extension.rust-nightly

To add insult to injury, those SDK extensions put cargo in different paths, and they have to be appended to PATH manually in the manifest.

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.

None yet

5 participants