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

Allow specifying paths for @library imports #3651

Merged
merged 1 commit into from Oct 20, 2023

Conversation

jpnurmi
Copy link
Collaborator

@jpnurmi jpnurmi commented Oct 11, 2023

This PR is a revised version of #3523 that drops everything related to Cargo and NPM, and instead allows manually specifying library paths in the compiler config:

// build.rs
use std::{collections::HashMap, env, path::PathBuf};

fn main() {
    let manifest_dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
    let library_paths = HashMap::from([
        (
            "libfile.slint".to_string(),
            manifest_dir.join("third_party/libfoo/ui/lib.slint"),
        ),
        (
            "libdir".to_string(),
            manifest_dir.join("third_party/libbar/ui/"),
        ),
    ]);
    slint_build::compile_with_config(
        "ui/main.slint",
        slint_build::CompilerConfiguration::new().with_library_paths(library_paths),
    )
    .unwrap();
}

Import types from the libraries:

import { Foo } from "@libfile.slint";
import { Bar } from "@libdir/components.slint";

export component MyApp inherits Window {
    Foo {}
    Bar {}
}

@jpnurmi

This comment was marked as outdated.

Copy link
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

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

Looks overall good to me! I think the error handling could be tweaked a little, but otherwise I love it.

Two other missing things I can think of:

  • An entry in the ChangeLog
  • Documentation in modules.md (could be done separately though)

api/napi/Cargo.toml Outdated Show resolved Hide resolved
internal/compiler/typeloader.rs Outdated Show resolved Hide resolved
),
if file_to_import.starts_with('@') {
format!(
"Cannot find requested library \"{file_to_import}\" in the library search path",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, but if the import is @some-library/some_file.slint the error message is going to call the entire string a library, when there are in fact two error cases:

  1. some-library could not be found (missing in library search path)
  2. Within some-library the file some_file.slint could not be found.

@jpnurmi
Copy link
Collaborator Author

jpnurmi commented Oct 18, 2023

Current status:

Import Library path Error
@libdir libdir=/path/to/lib Error reading requested import "/path/to/lib": Is a directory (os error 21)
@libdir/unknown.slint libdir=/path/to/lib Cannot find requested import "@libdir/unknown.slint" in the library search path
@libfile/unknown.slint libfile=/path/to/lib.slint Cannot find requested import "@libfile/unknown.slint" in the library search path
@unknown N/A Cannot find requested import "@unknown" in the library search path
@unknown/lib.slint N/A Cannot find requested import "@unknown/lib.slint" in the library search path

The second and third could be better.

@tronical
Copy link
Member

Current status:

Looks good to me. This makes a clear distinction at least between the library search path and regular includes.

Copy link
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

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

I think this is the way to go. This is a very good incremental step to me, and there's time for docs. I'd love to hear what the others are thinking, but IMO this is good to go.

@tronical
Copy link
Member

(please rebase once to get rid of the merge commit and the conflicts - we normally do rebases instead of merge, so no worries there :)

@jpnurmi
Copy link
Collaborator Author

jpnurmi commented Oct 18, 2023

I didn't include the docs from #3523 because they were so centered around the Cargo and NPM integration, which was left out of this PR. :)

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I think this could be merged as is, but still feel it is lacking a bunch of things:

Documentation: there seems to be almost nothing, but the concept of @library should be explained in the docs with help on how to pass these library path.

The slint-viewer also need a way to set the library path through command line argument. Currently there is -I for the include path, but it should also support something like -I library=/path/to/library or some other --lib library=/path/to/library
And this should be documented in the viewer docs

Same for slint-compiler (so it can be exposed in C++)

Maybe the documention of rust can make an example where a crate is taken as a build_dependency and do

pub fn slint_path() -> &str { env!("CARGO_MANIFEST_PATH/index.slint") }

api/rs/build/lib.rs Show resolved Hide resolved
@jpnurmi
Copy link
Collaborator Author

jpnurmi commented Oct 20, 2023

I've added an example to CompilerConfiguration::with_library_paths() docs and made it possible to pass library paths from the CLI. How do you like -Lexample=/path/to/lib?

Comment on lines +118 to +122
/// let manifest_dir = std::path::PathBuf::from(std::env::var_os("CARGO_MANIFEST_DIR").unwrap());
/// let library_paths = std::collections::HashMap::from([(
/// "example".to_string(),
/// manifest_dir.join("third_party/example/ui/lib.slint"),
/// )]);
Copy link
Member

Choose a reason for hiding this comment

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

This is not really what i had in mind.
I was thinking that the code that use CARGO_MANIFEST_DIR would be in a build-dependency (eg, coop_widget uploaded to crates.io)
but that work too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have somewhat mixed feelings about teaching users to wire up Cargo dependencies manually if the end goal is to make it automatic once the issues with compiling libraries have been sorted out. :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, i understand. Just leave it like this then.

tools/viewer/main.rs Show resolved Hide resolved
@ogoffart
Copy link
Member

How do you like -Lexample=/path/to/lib?

I like it. At least seems reasonable and can't think of anything better.

@jpnurmi
Copy link
Collaborator Author

jpnurmi commented Oct 20, 2023

Rebased and squashed the fixup commits

@FloVanGH
Copy link
Member

@jpnurmi this is the branch where I make usage of the @ https://codeberg.org/flovansl/co_sl/src/branch/coop_widgets/lib-import

@jpnurmi
Copy link
Collaborator Author

jpnurmi commented Oct 20, 2023

@jpnurmi this is the branch where I make usage of the @ https://codeberg.org/flovansl/co_sl/src/branch/coop_widgets/lib-import

Cool, thanks for giving it a try! A helper function for the import paths is quite nice to avoid an awkward empty lib.rs that I had in some earlier attempt. :)

@FloVanGH
Copy link
Member

@jpnurmi this is the branch where I make usage of the @ https://codeberg.org/flovansl/co_sl/src/branch/coop_widgets/lib-import

Cool, thanks for giving it a try! A helper function for the import paths is quite nice to avoid an awkward empty lib.rs that I had in some earlier attempt. :)

Thank you for implementing it ;-) . I definitely a good progress for the lib imports :-). I like it.

@tronical tronical merged commit c5248c0 into slint-ui:master Oct 20, 2023
32 checks passed
@jpnurmi jpnurmi deleted the library-paths branch October 20, 2023 14:50
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

4 participants