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

Respect Cargo target directory configuration #159

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Conversation

tyoeer
Copy link
Contributor

@tyoeer tyoeer commented Jul 17, 2023

Changes:

  • Removes hard-coded target/server and target/front
    • Using the normal target directory allows for sharing build dependencies between projects, which can significantly reduce compile times
    • It is unclear why they're hardcoded separate directories in the first place
    • Removing them was easier than making them use the proper target directory
  • Uses proper relative paths for bin and lib packages
  • Defaults site-root to the /site/ subfolder of the cargo target directory
    • That is insofar I can tell how it was intended
    • The current method is a bit clunky, but it's the best I could come up with with serde's default
      • An alternative would be to use an option and set it to Some in parse(),
        but that would forces Some checks everywhere.
    • Snapshot testing survives this because the test cases always manually specify it.

Didn't manage to add additional regression tests:

  • Changing the target directory has to be done through a .cargo/config.toml from where cargo is invoked,
    and does not take the --manifest-path into account
  • The tests already specify a cwd, though it'sn't used for the cargo metadata call,
    and isn't set to the directory of the actual project/workspace being tested. I can't figure out what exactly it's supposed to do,
    and I don't feel confident doing something with it.

Additional notes/things to look at that are outside the scope of this PR:

  • Snapshot testing is broken for developers who have a different Cargo target directory
  • clippy has complaints about cloning double references in src/ext/path.rs

@tyoeer
Copy link
Contributor Author

tyoeer commented Jul 21, 2023

Had to temporarily remove my own target-dir configuration to make it match the CI, but the tests look like they should match what the CI is expecting now.

Cargo.lock Outdated Show resolved Hide resolved
src/config/bin_package.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@aperepel aperepel 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 for this PR. I've added some suggestions, the primary one being to add support for the synonym var name as per https://doc.rust-lang.org/cargo/reference/config.html#buildtarget-dir

As a general comment, could you please include a sample dev experience walkthrough of how to leverage these improvements? This way I don't have to guess if I did the right set up to verify changes are doing what they are intended to accomplish.

src/config/lib_package.rs Outdated Show resolved Hide resolved
src/config/project.rs Outdated Show resolved Hide resolved
src/config/project.rs Outdated Show resolved Hide resolved
src/config/project.rs Outdated Show resolved Hide resolved
src/config/project.rs Outdated Show resolved Hide resolved
- Replace hard-coded strings with constant
- Add "CARGO_BUILD_TARGET_DIR" synonym
@tyoeer
Copy link
Contributor Author

tyoeer commented Aug 13, 2023

As a general comment, could you please include a sample dev experience walkthrough of how to leverage these improvements? This way I don't have to guess if I did the right set up to verify changes are doing what they are intended to accomplish.

@aperepel
The easiest way to test everything would be to configure the target directory in the top-level cargo configuration in ~/.cargo/config.toml, for example with:

[build]
target-dir = ".cargo/targetShare"

This shares the target directory between projects. I personally do this to speed up compilation and save disk-usage by re-using already compiled dependencies (between projects), other people might have other reasons.

With this configuration, one would expect that:

  • No panics happen
    • Previously it would panic for target directories outside of the workspace (see linked issues)
  • By default, the server binary gets built in the configured target directory, and can reuse compiled dependencies
    • This is configurable, though previously it would default to target/server, irregardless of the configured target directory, effectually overwriting your config for no apparent reason.
  • The wasm/client binary gets built in the configured target directory (though in the wasm32-unknown-unknown subfolder), and can reuse compiled dependencies
    • Previously, this was hard-coded to target/front, with no way to configure/change it, , effectually overwriting your target directory config for no apparent reason.
  • By default, the other site assets are put somewhere in the configured target folder
    • Previously, this would default to target/site, irregardless of the configured target directory, effectually overwriting your config for no apparent reason.

(Ideally there'd be a test demonstrating all this, but see my original PR comment for why I didn't manage to make one)

@tyoeer tyoeer requested a review from aperepel August 31, 2023 13:05
@Maneren
Copy link
Contributor

Maneren commented Oct 1, 2023

Hello, what's the status here? I've been using a binary compiled from this PR for quite some time now (without any issues) and I'd love to see this merged so I can use other updates as well. Is there anything I can help with? (code, testing, review, ...)

@gbj gbj merged commit 15535b2 into leptos-rs:main Oct 3, 2023
@gbj
Copy link
Contributor

gbj commented Oct 3, 2023

Sorry for the delay here — I've been reviewing PRs post-0.5 and am going to start going through these now.

@Maneren
Copy link
Contributor

Maneren commented Oct 4, 2023

No worries, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants