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

Migrate repo to a Cargo workspace #284

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Migrate repo to a Cargo workspace #284

merged 1 commit into from
Nov 28, 2023

Conversation

emwalker
Copy link
Collaborator

@emwalker emwalker commented Nov 26, 2023

This PR migrates the gosub-engine repo to a Cargo workspace. Discussion thread here.

2023-11-26_15-23

The idea is that we start out without changing much. The first crate is the gosub-bindings crate, and it goes under a new crates/ directory. Gradually, as we add more code and split other stuff off into their own crates, they'll also go under crates/. Eventually we might end up with a setup like Bevy's.

@jaytaph
Copy link
Member

jaytaph commented Nov 26, 2023

This will be fun rebasing ;-)

Would it make sense though to have certain other components in the engine (dns, config store) in separate crates as well, or should we keep them as-is for now until they outgrow the engine?

@emwalker
Copy link
Collaborator Author

@jaytaph I think the PR will be a lot less ambitious when I move it out of draft.

Eventually I think it makes sense to split out components into separate crates (still in this repo, though). But there's no hurry to do that. We should probably wait until there's a need that prompts it in each case rather than doing it preemptively.

@@ -12,19 +12,19 @@ bench: ## Benchmark the project
build: ## Build the project
source test-utils.sh ;\
section "Cargo build" ;\
cargo build
cargo build --all
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need the --all flag now to pick up gosub-bindings. So perhaps good to use make build now.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could add a target to build only one specific. But it might make sense to do this as a seperate PR


test_commands:
cargo run --bin html5-parser-test >/dev/null
cargo run --bin parser-test >/dev/null
cargo run --bin config-store list >/dev/null
cargo run --bin gosub-parser ./tests/data/tree_iterator/stackoverflow.html >/dev/null
cargo run --example html5-parser >/dev/null
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While we're here, let's add two more binaries to the CI run.

@emwalker emwalker marked this pull request as ready for review November 26, 2023 22:40
@emwalker emwalker requested a review from a team November 26, 2023 22:41
@@ -12,19 +12,19 @@ bench: ## Benchmark the project
build: ## Build the project
source test-utils.sh ;\
section "Cargo build" ;\
cargo build
cargo build --all
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could add a target to build only one specific. But it might make sense to do this as a seperate PR

@emwalker
Copy link
Collaborator Author

emwalker commented Nov 27, 2023

Maybe you could add a target to build only one specific. But it might make sense to do this as a seperate PR

Personally, I think keeping crate build --all follows the principle of least surprise, since without it you could end up breaking the crates under crates/ with a code change without knowing it, until someone attempts to build a specific crate you accidentally broke.

Running cargo test --all probably mitigates this situation almost entirely, since I think a debug build happens in the process. I'm not attached to what is in this PR, so feel free to change it in a later PR if it will save a lot on the build time.

Copy link
Member

@Kiyoshika Kiyoshika left a comment

Choose a reason for hiding this comment

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

LGTM

@emwalker emwalker merged commit 5b00c12 into main Nov 28, 2023
4 checks passed
@emwalker emwalker deleted the workspace branch November 28, 2023 00:54
@Sharktheone
Copy link
Member

Personally, I think keeping crate build --all follows the principle of least surprise, since without it you could end up breaking the crates under crates/ with a code change without knowing it, until someone attempts to build a specific crate you accidentally broke.

Running cargo test --all probably mitigates this situation almost entirely, since I think a debug build happens in the process. I'm not attached to what is in this PR, so feel free to change it in a later PR if it will save a lot on the build time.

I see what you mean. It could reduce the compile time when you have multiple changes. Mostly you only need to build one target. I mean, when you want to test the engine itself, you mostly don't need to build the bindings. When you have changes in other crates, for example when we have a dns crate, cargo also builds this because it has changes. In fact, you can't even compile the crate as a executable because it's a library crate.

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.

4 participants