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

feat: add resource resolver crate #106

Merged
merged 24 commits into from
Jan 23, 2024

Conversation

wiiznokes
Copy link
Contributor

same as #105 but with separate commits on cargo-packager.

Also, no cargo update has been run this time so let's hope this pass CI 🙏

@amr-crabnebula
Copy link
Collaborator

amr-crabnebula commented Dec 18, 2023

I am not too keen about this tbh because we have to support Node.js (as well as future bindings) and since the implementation is simple enough, it could be layed out as a spec in the documentation and users build their own resolver. cc @lucasfernog-crabnebula

@lucasfernog-crabnebula
Copy link
Member

the implementation isn't that simple due to security risks. I think we should have a crate like this (with bindings too, yay).
but then it also needs a way to resolve resources on development like what tauri has.. it's tricky.

@amr-crabnebula
Copy link
Collaborator

I think we can document the security risks and provide a reference example, also the implementation in the PR requires the app to be built by the packager and depends on feature flags which won't work in the bindings. The alternative would be to pass the format as an argument but I think it is better just provide a reference example.

@wiiznokes
Copy link
Contributor Author

What about documenting what env variables need to be set to use this crate (currently CARGO_PACKAGER_MAIN_BINARY_NAME and CARGO_PACKAGER_FORMAT), and let the user use the impl of this crate ?
It will always be more practical than implementing ourselves for each format, therefore documenting each specificity of each format.
Additionally, for apps that can be built with cargo-packager, it will work out of the box.

and depends on feature flags which won't work in the bindings

I'm not very familiar with bindings and tauri, can you explain what's causing the problem?

@amr-crabnebula
Copy link
Collaborator

What about documenting what env variables need to be set to use this crate (currently CARGO_PACKAGER_MAIN_BINARY_NAME and CARGO_PACKAGER_FORMAT), and let the user use the impl of this crate ?

Even if you set the CARGO_PACKAGER_FORMAT when building manually or though cargo-packager, you'd need to build the app multiple times, one for each package format so the resolver can work correctly.

Additionally, for apps that can be built with cargo-packager, it will work out of the box

They would still need to build the app multiple times using beforeEachPackagingCommand

It will always be more practical than implementing ourselves for each format, therefore documenting each specificity of each format.

I am not totally opposed to the point but I am just trying to make sure we cover the needed cases.

and depends on feature flags which won't work in the bindings

I'm not very familiar with bindings and tauri, can you explain what's causing the problem?

The bindings I mentioned are Node.js bindings for the packager and updater which allows usage of these crates in Node.js and Electron environments, which can be found here https://github.com/crabnebula-dev/cargo-packager/tree/main/bindings.

@wiiznokes
Copy link
Contributor Author

The alternative would be to pass the format as an argument

This seems reasonable, if all formats provide this feature. But it could perhaps cause problems when the program is invoked by command line.

but I think it is better just provide a reference example.

From my pov, this will just move the problem to the user, how will he know what format is in use?

Even if you set the CARGO_PACKAGER_FORMAT when building manually or though cargo-packager, you'd need to build the app multiple times, one for each package format so the resolver can work correctly.

I don't really see this as a problem, because most of the time, the package build step is done during a release. The dev can use cargo run as usual. This remains my favorite solution atm. Besides, it's not like we have to rebuild all the deps, just 2 crates.

@amr-crabnebula
Copy link
Collaborator

amr-crabnebula commented Jan 2, 2024

The alternative would be to pass the format as an argument

This seems reasonable, if all formats provide this feature. But it could perhaps cause problems when the program is invoked by command line.

Argument here is an argument to the resolve_resource function not a cli argument (or did I misunderstood you?)

but I think it is better just provide a reference example.

From my pov, this will just move the problem to the user, how will he know what format is in use?

We can't know either, there is a better chance the user knows which version he is using.

Even if you set the CARGO_PACKAGER_FORMAT when building manually or though cargo-packager, you'd need to build the app multiple times, one for each package format so the resolver can work correctly.

I don't really see this as a problem, because most of the time, the package build step is done during a release. The dev can use cargo run as usual. This remains my favorite solution atm. Besides, it's not like we have to rebuild all the deps, just 2 crates.

You can't guarantee that the user will use beforeEachPackagingCommand or set up the env var himself and ofc can't support Node.js bindings and so I don't really like the env var approach and would instead go for the function argument approach.

You don't need to worry about the bindings, I can add them later if you can't

@wiiznokes
Copy link
Contributor Author

wiiznokes commented Jan 2, 2024

Argument here is an argument to the resolve_resource function not a cli argument (or did I misunderstood you?)

Oh, I thought you were speaking about cli argument, mb.

From my pov, this will just move the problem to the user, how will he know what format is in use?

We can't know either, there is a better chance the user knows which version he is using.

Imagine you want to use both deb and rpm (i know rpm is not yet supported), this will not be easy/straitforward.

You can't guarantee that the user will use beforeEachPackagingCommand or set up the env var himself and ofc can't support Node.js bindings and so I don't really like the env var approach and would instead go for the function argument approach.

I don't really understand why the bindings can't work in this case? Isn't the resource-resolver crate be compiled in the same environnement?

What about having the main API use an arguments, like you said, and providing a feature (not default), without node.js binding (if it's possible, idk), that provide the current_format fonction, with a big warning about the requirement

  • build with cargo-packager and before-each-package-command atribute
  • or the user pass a documented CARGO_PACKAGER_FORMAT env variable.

This will allow a user to not use a build.rs file, which can be unintuitive for a beginer, while also having an out of the box solution for resource, for rust app.

For the deb implementation, maybe CARGO_BIN_NAME from cargo specific env vars can be used instead of CARGO_PACKAGER_MAIN_BINARY_NAME, but i'm not sure.

@amr-crabnebula
Copy link
Collaborator

Imagine you want to use both deb and rpm (i know rpm is not yet supported), this will not be easy/straitforward.

For us, we can't really detect the currently installed package format because the user is the one responsible for building their app and so they can embed this metadata while we can't.

I don't really understand why the bindings can't work in this case? Isn't the resource-resolver crate be compiled in the same environnement?

it is not the same, because it will be pre-compiled in our CI as a node native addon (NAPI) and be pulled from NPM to users.

What about having the main API use an arguments, like you said, and providing a feature (not default), without node.js binding (if it's possible, idk), that provide the current_format fonction, with a big warning about the requirement

  • build with cargo-packager and before-each-package-command atribute
  • or the user pass a documented CARGO_PACKAGER_FORMAT env variable.

This will allow a user to not use a build.rs file, which can be unintuitive for a beginer, while also having an out of the box solution for resource, for rust app.

For the deb implementation, maybe CARGO_BIN_NAME from cargo specific env vars can be used instead of CARGO_PACKAGER_MAIN_BINARY_NAME, but i'm not sure.

I guess that's the only way to it really (however it may fragment the usage a bit).

@wiiznokes
Copy link
Contributor Author

Nsis, Deb and cargo run work for me, and idk what's prettier have against me.

I'm not sure current_exe will always works for debian tho, maybe taking the app name as an arg is better, but it's will be a shame, since it's the only format that require it.

crates/resource-resolver/src/starting_binary.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/README.md Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/build.rs Outdated Show resolved Hide resolved
crates/resource-resolver/Cargo.toml Outdated Show resolved Hide resolved
crates/packager/src/package/mod.rs Outdated Show resolved Hide resolved
@denjell-crabnebula
Copy link
Contributor

@naman-crabnebula - would you mind reviewing the deb process here?

@denjell-crabnebula
Copy link
Contributor

also looks like a merge from main is needed @wiiznokes

@lucasfernog-crabnebula
Copy link
Member

I'll take a look at this over the weekend, I love this idea :)

crates/resource-resolver/src/error.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/error.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/src/lib.rs Outdated Show resolved Hide resolved
crates/resource-resolver/Cargo.toml Outdated Show resolved Hide resolved
@wiiznokes
Copy link
Contributor Author

I didn't tested appImage

amr-crabnebula
amr-crabnebula previously approved these changes Jan 23, 2024
Copy link
Collaborator

@amr-crabnebula amr-crabnebula left a comment

Choose a reason for hiding this comment

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

Thank you and sorry for the long delay

@amr-crabnebula amr-crabnebula changed the title add Resource resolver crate feat: add resource resolver crate Jan 23, 2024
@amr-crabnebula amr-crabnebula linked an issue Jan 23, 2024 that may be closed by this pull request
@amr-crabnebula amr-crabnebula merged commit 67e55f6 into crabnebula-dev:main Jan 23, 2024
23 checks passed
@wiiznokes wiiznokes deleted the resource-resolver branch February 9, 2024 20:49
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.

How to load ressource at runtime?
4 participants