Allow deploy to a custom location
#98
Conversation
src/deployment.rs
Outdated
| package: &CargoPackage, | ||
| target: &CargoTarget, | ||
| result: &CargoResult, | ||
| mut with_serve_path: Option<DeployWithServePath> |
There was a problem hiding this comment.
Please make this function take serve_path: String instead of with_serve_path. There is no point in making anything here Optional since both deploy_path and serve_path have well defined defaults we can use (one being target/deploy, the other /), so it makes no sense to overcomplicate the code.
There was a problem hiding this comment.
tldr:
It is complicated because I want to check if we need to process the js contents or not. If serve_path is default or there is no .wasm file: do nothing on js contents, else, must process it. Another reason is I am too lazy (:-1: to myself) to add more code (than a None) to cmd_start.rs.
Long story: (Please correct me if I say something not correct, and apologies for my poor English)
This is because of the involvement in cmd_start.rs (line 146),
let deployment = Deployment::new( &package, &target, &result, None )?;If don't want to take an Option, then it will become (None would be better?)
let config = project.get_default....
let deployment = Deployment::new( &package, &target, &result, config )?;I think it is ok with that. But there is a more important thing to consider is that we must process the contents of the js file. If I understand correctly, we do not need to process it in case of asmjs, is this right? (don't know this exactly because I do not use asmjs) (and do not process the js file if serve-path is default) So, I add DeployWithServePath to monitor if there is a .wasm file and to store its actual name (on the filesystem, and that is also use in the js). Then when process the js contents, we check if it is required or not, if not, just do nothing. Otherwise, find the .wasm filename (recorded by DeployWithServePath) in js file, insert serve-path.
If you want to pass serve-path as a String, then we need a second parameter that indicate if target is a wasm32 or not (If it is not, we do not process the js contents), and it will become something like this:
pub fn new(
package: &CargoPackage,
target: &CargoTarget,
result: &CargoResult,
serve_path: String, // this
target_is_wasm: bool, // and this
) -> Result< Self, Error >Finally, with the fn declaration above, we check if it is default or not by comparing serve_path.len() == 0 to know if it is needed to process or not process the js contents. If we pass as Option<String> it is clearer if we are in the default case or not.
There was a problem hiding this comment.
But there is a more important thing to consider is that we must process the contents of the js file.
Hm, yeah, you're right that we need to mess around with the .js file when serve-path != "/".
asmjs-unknown-emscripten- nothing to dowasm32-unknown-emscripten- we need to replace the path to the.wasmfile or (if possible) tell Emscripten to use the path we want right from the start (it might be possible to pass that info to Emscripten throughEMMAKEN_CFLAGS, I don't know)wasm32-unknown-unknown- we need to generate the.jsfile with the right path
I think that doing this during building (instead of adding hacks to change the path during deployment) would be simpler.
There was a problem hiding this comment.
What I tried is actually hacking the .js file 👎! The reason is that I don't want to touch any code outside of the deploy process. I think that putting any hacky things into DeployWithServePath then if we have full support for serve-path in both wasm32-unknown-emscripten and wasm32-unknown-unknown just remove it all.
But I understand that it is not good for the consistency of the whole project! I just don't have much time to dive deeper into build process and especially into Emscripten right now (even you have to say "I don't know"). So, let this halted here for awhile, until I have some free time to devote more into this.
There was a problem hiding this comment.
Well, initially I would be fine with just doing a search and replace for wasm32-unknown-emscripten; as long as it works we can always fix it later. The initial version of this doesn't have to be perfect, but I'm willing to accept only minor hacks as opposed to major ones. (:
Sure, no problem, thanks for investing the time into this. (:
src/deployment.rs
Outdated
| // It is not allow to contains `\\` or `..` | ||
| if path.contains( &double_sep ) || path.contains( ".." ) { | ||
| return Err( Error::ConfigurationError( format!("serve-path is invalid: {}", path) ) ); | ||
| } |
There was a problem hiding this comment.
The validity of these should all be checked when the config file is loaded.
There was a problem hiding this comment.
Yes, it is better to do this when loading config
src/deployment.rs
Outdated
| } | ||
| } | ||
| _ => unreachable!() | ||
| } |
There was a problem hiding this comment.
This looks a little too complicated; we shouldn't (mostly) need these functions, I think.
It would be simpler to just to do something like this:
fn rebase_url( serve_path: &str, url: &str ) -> String {
// ...
}and then:
for path in result.artifacts() {
let (is_js, key) = match path.extension() {
- Some( ext ) if ext == "js" => (true, js_name.clone()),
+ Some( ext ) if ext == "js" => (true, rebase_url( &serve_path, &js_name )),
- Some( ext ) if ext == "wasm" => (false, path.file_name().unwrap().to_string_lossy().into_owned()),
+ Some( ext ) if ext == "wasm" => (false, rebase_url( &serve_path, path.file_name().unwrap().to_string_lossy() )),
_ => continue
};There was a problem hiding this comment.
As I mentioned this in my first reply above. We need to know the actual name of the .wasm file (its name only, not include the serve_path, because what we want to insert the serve_path into js, before its actual name).
src/cmd_deploy.rs
Outdated
| let target_config = match project.config_of_default_target() { | ||
| Some(config) => config.clone(), | ||
| None => ::config::PerTargetConfig::default() | ||
| }; |
There was a problem hiding this comment.
Instead of this please add deploy_path and serve_path getters to Project. Have them always return String.
There was a problem hiding this comment.
I agree with the deploy_path. But with serve_path, I think return Option<String> is better, because we do different things if there is a serve-path or not.
There was a problem hiding this comment.
How do we do different things if there is a serve-path? By default the behavior should be the same as if serve-path was set to /, no?
|
As far as the standalone tests go you need to do something like this to run them locally: Running |
|
Oh no, I added Edit: |
| let raw: toml::Value = match toml::from_str( config_toml.as_str() ) { | ||
| Ok(value) => value, | ||
| Err(error) => return Err( Error::ConfigurationError( | ||
| format!( "Failed to parse Web.toml: {}", error ) |
There was a problem hiding this comment.
Can you format this error the same way as other errors generated here? e.g. format!( "cannot parse {}: {}", config.source(), error ).into() Also, not much point in putting it into Error::ConfigurationError (I'll probably get rid of those variants.)
| "deploy-path" => if is_main_crate { | ||
| config.add_deploy_path(ALL_BACKENDS, toplevel_value, "deploy-path")?; | ||
| }, | ||
| "js-wasm-path" => if is_main_crate { |
| "js-wasm-path" => if is_main_crate { | ||
| config.add_js_wasm_path(ALL_BACKENDS, toplevel_value, "js-wasm-path")?; | ||
| }, | ||
| "serve-url" => if is_main_crate { |
There was a problem hiding this comment.
serve-url -> serve-path (What we're specifying there are not URLs, e.g. URLs have scheme followed by a semicolon.)
| return Err( Error::ConfigurationError( format!("serve-url is invalid: {}", path) ) ); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
- The serve path should:
- not contain more than two consequtive
/nor..(you're already checking for this) - not contain any
\(they should always use forward slashes, so we shouldn't useMAIN_SEPARATOR)
- Please keep the format of the error messages consistent with others.
| let double_sep = format!("{0}{0}", SEP); | ||
| if path.contains( &double_sep ) || path.contains( ".." ) { | ||
| return Err( Error::ConfigurationError( format!("js-wasm-path is invalid: {}", path) ) ); | ||
| } |
There was a problem hiding this comment.
Same issues as in serve path validation.
| # The default value of `js-wasm-path` is "/" (means the root of `deploy-path`) | ||
| js-wasm-path = "js/and/wasm/folder/inside/deploy/path" | ||
|
|
||
| # [Optional] If ommitted, the value of `serve-url` will be the same as `js-wasm-path` |
There was a problem hiding this comment.
The path under which the `.js` file will be served; by default equal to `/`.
In general I'd like the serve-path to be the "main" one, instead of how is it now where it's the other way around. (Also remember that with the asmjs target we don't even have a .wasm file.) Basically:
- By default put both
.jsand.wasmunderserve-path. - If the user wants to put the
.wasmfile somewhere else then they can setwasm-path. - The path to the
.wasmfile in the.jsfile will be always consistent with whatcargo web startis serving andcargo web deployis outputting.
I know there's also the use case of deploying the .js or .wasm to a different directory than the path from which they're served (e.g. if you're rewriting URLs in your webserver), but I'd rather handle that separately in a future PR.
| # If specified, the folder must exist. | ||
| deploy-path = "path/to/deploy/folder/" | ||
|
|
||
| # The default value of `js-wasm-path` is "/" (means the root of `deploy-path`) |
There was a problem hiding this comment.
The path from which the `.wasm` file will be served; by default the same as `serve-path`.
- Put it after
serve-path.
| deploy-path = "path/to/deploy/folder/" | ||
| js-wasm-path = "js/and/wasm/folder/inside/deploy/path" | ||
| serve-url = "url/to/retrieve/js/and/wasm/files" | ||
| ``` |
There was a problem hiding this comment.
Please merge this example Web.toml into the main one. (So that everything is in a single place.)
|
|
||
| [target.wasm32-unknown-unknown] | ||
| # If you specify these parameters for a specific target then global `deploy-path`, | ||
| # `js-wasm-path` and `serve-url` are not allowed. |
There was a problem hiding this comment.
This comment here is unnecessary; instead we can extend the list of keys in A few restrictions concerning paragraph higher in the README.
| If you have your wasm-app files and other static files serve like | ||
| the above example. Please notes: | ||
| * An `index.html` must be provided in `crate-root/static` and have it point to | ||
| the right `url` of `.js` file for browsers to be able to load it. |
There was a problem hiding this comment.
Hm... with good enough one/two line comments in the example Web.toml file I feel like this long winded explanation might not be necessary? (Expect the warning about the files being overwritten, which should be spelled out separately.)
|
Thanks for your review! Why do I use
|
You can just simply run
Well, the idea is that I agree that it's probably rare that someone wants to keep the
Mostly because this PR is already very big, and in most cases it probably won't be necessary. (: Having a different path for deployment does bring some additional complexity (e.g. we'll need to add forced rebuilding to be able to patch the filename again if someone first does
So basically - disentangle the paths to However, I'd still like to keep the invariant that
There are two main problems with absolute paths: 1) they're OS specific (e.g. Windows uses drive letters, Linux doesn't), and 2) they effectively make the crate unusable if you, for example, copy your project to a different directory or a different drive. So I don't think it'd be really worth supporting them? (And I don't think encouraging (2) is a good idea.) |
|
After some long thought about this, I think we should make things simpler by
First step: close this PR 😁
Second: A new PR only for
|
|
Sounds mostly good to me, except a few points:
Am I misunderstanding what you just said, or are you suggesting that we should serve everything under the The static files from the
Unless you're talking purely conceptually here, internally there is no need to do any fiddly stripping; the files should just be mounted under just prepend the This will automatically make the file appear in the same place with both
Well, I'm fine with supporting it as long as someone actually needs this feature. |
|
This will go into detail, I just want to make sure that we are talking and thinking about exactly the same thing! Pretend we a going to build a site name The rust source code that will be deployed by In deploy-path = "/path/to/rusters-static"Then after Because of the complexity of the project, we want our server serves things like this: Our server code similar to: let server = Server::new()
.index(serve_index_html)
.route("users", handle_users)
.route("posts", handle_posts)
....
.route("s-files", StaticFiles::new("/path/to/rusters-static")); // under `s-files`This way, what we expect are: (all of them have Although we serve-path = "s-files"If we prepend let key = format!( "{}/{}", serve_path, key );Then, With Maybe we could say we are running into a recursive problem?? I can only figure out two solutions for this problem:
struct Route {
serve_key: String, // use for `cargo web start`
deploy_key: String, // use for `cargo web deploy`
} |
|
After we set the deployment (both If the user wants to have the Or to reiterate:
Sorry, I don't fully understand what's the problem here. (: We prepend the |
Maybe you missed thisIn this comment, maybe you missed the following line?
If you missed that line, I think I should say it more clearly that:
There are only just two files (.js and .wasm), I think it is not a big problem if we force them into the root of deploy path. (Except for We are still talking about different things:
Yes, if we serve things that way, it's ok with the very first version of I said about a different case, where users want to put them under a route. I said it two times, and somehow you missed both. The first time in this comment, that: The second time in this comment, that: Maybe a simpler solution?Here, wasm32-unknown-emscriptenLeave it as is. I read all emscripten`s flags/settings about 3 for 4 times but I am unable to find anything that can change the url to load wasm file from js file. Maybe we should open an issue on emscripten? Wait until emscripten support it? wasm32-unknown-unknownInstead of hard-code const STDWEB_WASM_PATH = "/<name>.wasm";Then the This way, most users just need a normal Using this solution: |
|
@limira Apologies for the late reply.
Sorry, I was a little confused, but I think I get it now... (: Just to double check, by this:
... do you mean you want the serve paths to affect only two things:
but when running Hmmm.... this is starting to get messy. Okay, so, can we for now have a PR with only the |
|
At @koute's invitation I'd like to register a related feature request. I need to, ideally using Web.toml, specify a host and path for the generated .js to use when fetching the .wasm file. I also need to have the option to customize the specified host and path depending on the environment I'll deploy the build to and serve it from. (eg., development, production) Example using the current behavior in development and serving from a CDN in production:
|
|
I just come to say that I am currently unable to continue the work on this PR. I don't know if I may come back again to finish this or not. Thank you for all your time spending to review my work! |
|
@limira Thanks for all of your work! I'll close this PR for now; if someone wants to resurrect this feel free to open a new one! @kellytk I've added a new argument to the current For example, you can do something like this: import factory from "./module.js";
const instance = factory();
fetch( "http://example.com/url/to/your/module/module.wasm" )
.then( response => { return response.arrayBuffer(); } )
.then( bytes => { return WebAssembly.compile( bytes ); } )
.then( mod => { return WebAssembly.instantiate( mod, instance.imports ) } )
.then( wasm_instance => {
const exports = instance.initialize( wasm_instance );
exports.functionExportedWithTheJsExportMacro();
});Basically, the generated {
initialize: function( wasm_instance ) { ... },
imports: { ... }
}So while this might not be as ergonomic as having this in |
Solve issue #96
Basically, this work. But no code for auto-test yet! Reading the code, I am unable to figure out how to run test locally!
How can I run tests locally? Especially, with a custom deploy-path?