Support workspaces at library level #197

AnthonyMichaelTDM · 2023-04-02T03:27:20Z

Partially addresses #194

What this does:

Adds library level support for users to use a virtual manifest (a Cargo.toml with a [workspace] section, but not a [package] section) for their project's root Cargo.toml to separate their backend from the helper binaries (such as fullstack).
by default, supports a project structure similar to the one used in Use Cargo Workspaces in generated project to reduce compilation times #194, however there are environment variables that allow users to use a more customized project architecture

What this doesn't do

Add functionality to generate a project that uses a virtual manifest at the project root.

Bumps [openssl](https://github.com/sfackler/rust-openssl) from 0.10.45 to 0.10.48. - [Release notes](https://github.com/sfackler/rust-openssl/releases) - [Commits](sfackler/rust-openssl@openssl-v0.10.45...openssl-v0.10.48) --- updated-dependencies: - dependency-name: openssl dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ml of user projects

AnthonyMichaelTDM · 2023-04-02T19:57:33Z

~~Wait before merging!~~ (this was fixed a while ago)

Compilation is hanging for cargo fullstack, something to do with cargo-watch but either way I need to trouble shoot that to ensure it's not a problem with the code here.

AnthonyMichaelTDM · 2023-04-03T21:15:47Z

I think I figured out my compilation issue, but a fix will require adding a new feature so that create-rust-app::dev::run_server (and it's sub-processes) opperate slightly differently

Wulf · 2023-04-03T21:29:59Z

hey Anthony, this seems like a big change, please give me some time to review it and think about whether this added complexity is appropriate

Wulf · 2023-04-03T21:43:36Z

Is this the only relevant change? If so, do you mind creating a PR for this separately (or just move the rest of the changes elsewhere)? I'm okay with this

AnthonyMichaelTDM · 2023-04-03T21:59:29Z

Is this the only relevant change? If so, do you mind creating a PR for this separately (or just move the rest of the changes elsewhere)? I'm okay with this

currently yeah, but there are more things that need to be changed to get everything to work properly (currently hangs on compiling the backend if you run cargo fullstack in a project using a workspace layout), enough changes that I see it fit to add a new feature plugin_workspace_support

the other changes are miscellaneous bug-fixes (caught by running cargo clippy w/ and w/o debug-assertions enabled), formatting improvements (auto-generated by cargo fmt --all), and an optimized CI pipeline for the library and binary themselves (not the generated projects, that'll be done in a successor to #182 ) (CI.yml)

once everything is ready I'll be sure to do an in-depth code review that explains all the changes (like I have in the past for other big changes) but feedback is greatly appreciated in the meantime

If, when everything is ready, you'd still like me to split it into separate PR's I can do so then

AnthonyMichaelTDM · 2023-04-03T22:01:30Z

hey Anthony, this seems like a big change, please give me some time to review it and think about whether this added complexity is appropriate

for sure, let me set it as a draft while I figure things out on my end

AnthonyMichaelTDM · 2023-04-04T01:43:39Z

figured out it's not an issue with cargo-watch (or if it was, I fixed it already), it's likely being caused by bad file paths in various parts of the codebase, the working directory of the server binary is ./backend not ./ when using workspaces

gotta go, here's a list of bugs i've found so far and need to fix

the stuff that uses Tera (rendering templates) isn't finding the views folder
the logger seems to be broken (actix), though this could also be related to tokio

Wulf · 2023-04-04T11:12:12Z

Ah, yeah — there’s definitely lots of tech debt in the code base when it comes to file paths. You’ve probably seen some of the TODO statements. The logging situation also needs reconsidering — I’ve always wanted to implement a universal logger which is why some of the logging setup is in the helper crate (create-rust-app) instead of the generated app. Alongside this, do you have any thoughts on moving package.json and other front end stuff to the root level of the project? It really sucks to have to cd into frontend in order to execute some commands; however, it also allows the end user to , for example, introduce a third folder for a mobile app or something similar (and thereby potentially sharing some common code amongst the three projects).

AnthonyMichaelTDM · 2023-04-04T14:27:16Z

I personally like the flexibility granted by having the package.json in the frontend folder, it also gives us the opportunity to support rust frontends in the future

AnthonyMichaelTDM · 2023-04-04T19:11:41Z

And regarding the tech debt, I have an idea we could implement remedy it (at least for the file paths and urls):

Extract all the directory paths (such as the path to views, the project root, the backend root, frontend root, dist folder, etc.) into environment variables, while we're at it, we can do URL's too (brought up in [BUG] Hard-coded URL's should be set by environment variable(s) #75)
- Pros: easy to configure on end user's end, and we could give default values in the code so that the environment variables are not necessary for people who don't need them, but available for people who want them (this would mean it's not a breaking change, and won't necessarily require a semvar bump).
- Cons: could be a little cluttered, especially in the .env file, but in the code this clutter can be contained by adding a module full of global variables in the form of lazy statics, like paths.rs or something.

I'd like your thoughts on this though, because I might be missing something.

AnthonyMichaelTDM · 2023-04-16T00:37:57Z

I think I figured out the issue with logging, this line was recently commented out, when I add the code to my project's main fn the logging works again.

create-rust-app/create-rust-app/src/lib.rs

Line 93 in fd10180

    
           // env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("info")).init();

AnthonyMichaelTDM · 2023-04-18T01:12:46Z

@Wulf the fix I suggest is to add that line of code into the template for actix, as I assume it was removed for a reason

AnthonyMichaelTDM · 2023-05-03T05:04:25Z

I'll probably redo this soon

but it actually works this time, in addition to compiling

AnthonyMichaelTDM · 2023-05-29T03:56:24Z

Tested working on JARA dev

AnthonyMichaelTDM · 2023-05-29T04:11:04Z

With this addition, it might be approaching time to have an example's folder showing the 4 basic project types, as well as some more advanced setups (i.e., workspaces, different frontend frameworks / languages, mixing SQL (for auth) and NoSQL (for everything else) databases, etc.)

what do you think @Wulf

Wulf · 2023-09-27T01:59:00Z

Look what we have here 😅

Let's get this PR merged in soon :)

Wulf · 2023-09-28T02:30:16Z

create-rust-app/src/util/workspace_utils.rs

+    pub static ref WORKSPACE_DIR: PathBuf = {
+        let output = std::process::Command::new(env!("CARGO"))
+            .arg("locate-project")
+            .arg("--workspace")
+            .arg("--message-format=plain")
+            .output()
+            .unwrap()
+            .stdout;
+        let cargo_path = Path::new(std::str::from_utf8(&output).unwrap().trim());
+        cargo_path.parent().unwrap().to_path_buf()
+        // .to_str()
+        // .unwrap()
+        // .to_owned()
+    };


the rest looks good to me but here we're assuming we'll have the project's source code. That's not necessarily the case (see #332 -- we split the build and binary container into two). We also don't want to go in this direction as it stops us from attempting a "single binary" build in the future as well.

It does point out that there's some existing tech debt here that needs to be addressed

Should we have it stored in an environment variable instead? Similar to frontenddir

It's just that we need a way to know where the base of the project is and that's not always the current working directory

Yeah. Some more thoughts:

I think we need to remove the feature flag. In create-rust-app/src/dev/mod.rs, we should check if it's a workspace using the Cargo.toml instead of relying on a feature flag.

Also, all the variables in this file should be &'static str and be initialized to some default unless the backing environment variable is present (and they should all be configurable by env vars).

Let me know what you think!

I've implemented most of your suggestions as of the most recent commits (also got rid of some unnecessary unwraps), but there's a few things I still need to figure out:

a function that can tell us if we're using workspaces (without needing source code) (rather than using the feature flag)

a way of finding the workspace root when the (not yet added) CRA_WORKSPACE_ROOT environment variable isn't set

btw, sorry I've been awol for so long, I was busy at work over the summer and then school's been keeping me busy too, so I haven't had much time.

sounds awesome -- it's cool to hear about your journey so far :)

I'll have to take some time out to look into this further

I think I figured something out, high level is that I made "finding the workspace dir" infallible (no panics, even in a container or something), then rethought the logic used in initializing these variables (getting rid of the need for the feature flag)

@Wulf is this resolved?

beautifully done.

🙌

… can make a function that can tell us if we're using workspaces

…iable initialization logic in worspace_utils

dependabot bot and others added 4 commits March 26, 2023 00:14

Merge branch 'Wulf:main' into main

149af2c

support using workspaces with a virtual manifest at the root Cargo.to…

e65675f

…ml of user projects

clippy and fmt

c7fcd03

AnthonyMichaelTDM requested a review from Wulf April 2, 2023 03:27

AnthonyMichaelTDM self-assigned this Apr 2, 2023

AnthonyMichaelTDM marked this pull request as ready for review April 2, 2023 04:06

AnthonyMichaelTDM mentioned this pull request Apr 3, 2023

Ci fixes #182

Merged

CI.yml optimizations from #182

e32badb

clippy and fmt fixes from #182 and elsewhere

b15eb73

AnthonyMichaelTDM marked this pull request as draft April 3, 2023 22:02

Add workspace support feature

201ca5e

some additional comments

f23fd1f

AnthonyMichaelTDM added 2 commits May 28, 2023 16:22

Merge branch 'main' into support-workspaces

59eb497

support project organization as a workspace

5d1f900

but it actually works this time, in addition to compiling

AnthonyMichaelTDM marked this pull request as ready for review May 29, 2023 03:55

AnthonyMichaelTDM mentioned this pull request May 29, 2023

Maintenance, dependency updates #243

Merged

Merge branch 'main' into support-workspaces

be8ff07

Wulf requested changes Sep 28, 2023

View reviewed changes

Wulf force-pushed the main branch 2 times, most recently from 44511d0 to e23de14 Compare September 30, 2023 00:00

AnthonyMichaelTDM added 3 commits November 7, 2023 16:38

don't rely on feature flag in create-rust-app/src/dev/mod.rs

49c2536

refactor workspace_utils values to static strings

bdd2020

refactor workspace_utils vars so we can remove the feature flag if we…

9e32fd3

… can make a function that can tell us if we're using workspaces

AnthonyMichaelTDM requested a review from Wulf November 8, 2023 02:09

AnthonyMichaelTDM added 3 commits November 7, 2023 18:11

Merge branch 'main' into support-workspaces

81a92fa

remove need for plugin_worspace_support feature flag and overhaul var…

2d42480

…iable initialization logic in worspace_utils

refactor and test logic in workspace_utils.rs

0eaf0a2

Wulf approved these changes Nov 18, 2023

View reviewed changes

AnthonyMichaelTDM merged commit 107cd04 into Wulf:main Nov 19, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support workspaces at library level #197

Support workspaces at library level #197

AnthonyMichaelTDM commented Apr 2, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 2, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 3, 2023

Wulf commented Apr 3, 2023

Wulf commented Apr 3, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 3, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 3, 2023

AnthonyMichaelTDM commented Apr 4, 2023

Wulf commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 16, 2023

AnthonyMichaelTDM commented Apr 18, 2023

AnthonyMichaelTDM commented May 3, 2023

AnthonyMichaelTDM commented May 29, 2023 •

edited

Loading

AnthonyMichaelTDM commented May 29, 2023 •

edited

Loading

Wulf commented Sep 27, 2023

Wulf Sep 28, 2023 •

edited

Loading

AnthonyMichaelTDM Sep 28, 2023

Wulf Sep 29, 2023

AnthonyMichaelTDM Nov 8, 2023

AnthonyMichaelTDM Nov 8, 2023 •

edited

Loading

Wulf Nov 11, 2023

Wulf Nov 11, 2023

AnthonyMichaelTDM Nov 12, 2023

AnthonyMichaelTDM Nov 12, 2023

Wulf Nov 18, 2023

Support workspaces at library level #197

Support workspaces at library level #197

Conversation

AnthonyMichaelTDM commented Apr 2, 2023 • edited Loading

AnthonyMichaelTDM commented Apr 2, 2023 • edited Loading

AnthonyMichaelTDM commented Apr 3, 2023

Wulf commented Apr 3, 2023

Wulf commented Apr 3, 2023 • edited Loading

AnthonyMichaelTDM commented Apr 3, 2023 • edited Loading

AnthonyMichaelTDM commented Apr 3, 2023

AnthonyMichaelTDM commented Apr 4, 2023

Wulf commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 4, 2023

AnthonyMichaelTDM commented Apr 16, 2023

AnthonyMichaelTDM commented Apr 18, 2023

AnthonyMichaelTDM commented May 3, 2023

AnthonyMichaelTDM commented May 29, 2023 • edited Loading

AnthonyMichaelTDM commented May 29, 2023 • edited Loading

Wulf commented Sep 27, 2023

Wulf Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyMichaelTDM Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyMichaelTDM commented Apr 2, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 2, 2023 •

edited

Loading

Wulf commented Apr 3, 2023 •

edited

Loading

AnthonyMichaelTDM commented Apr 3, 2023 •

edited

Loading

AnthonyMichaelTDM commented May 29, 2023 •

edited

Loading

AnthonyMichaelTDM commented May 29, 2023 •

edited

Loading

Wulf Sep 28, 2023 •

edited

Loading

AnthonyMichaelTDM Nov 8, 2023 •

edited

Loading