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

WIP: Use xtask as build system #2012

Merged
merged 43 commits into from
Dec 17, 2022
Merged

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Dec 11, 2022

Moves the build system away from cargo make to cargo xtask.

Rationale

cargo make isn't an ideal buildsystem for zellij for a number of reasons:

  • Tasks by default run on each member of a workspace, member by member. Because we need to have the plugins built first (with the recent rework of the internal asset map in utils) this means that in fact we build the plugins about 10 times in a single cargo build
  • cargo make produces a lot of cargo make-specific output for little program output. This often makes it hard to follow what task exactly failed and on which workspace member
  • It is comparatively slow
  • It doesn't give a sensible overview of the available tasks to run

Also, the recent release showed that clearly we hit some boundary with cargo make and the way we're using it. So it is probably time to think about an alternative.

Why xtask

cargo xtask isn't so much a tool on its own as a member of the project workspace. It is a subcrate inside the project, which runs project-specific tasks. This has a great benefit: It needs no additional dependencies. cargo xtask is compiled as a rust binary and then parses all other arguments.

It is neither a recommended workflow, not a de-facto standard. The "website" can be found here: https://github.com/matklad/cargo-xtask

It is used by other influential rust projects, such as rust-analyzer. The benefit for zellij is that we can customize it more easily and adapt it to our changing requirements. This is especially important since with the plugin system we need to handle some added complexity otherwise missing in "regular" rust projects.

Current state

There are still a few things compared to cargo make that it isn't capable of (yet):

  • Accept arbitrary arguments: This is mostly interesting for cargo make run or cargo make test, where developers can currently pass any additional CLI args at the end, which are passed through to the application. I'm still looking for a way to do this...
  • The cargo make publish flow

Also I'd like to add another cargo alias/deprecation warning for users still using cargo make to inform them of the new build system.

@har7an har7an temporarily deployed to cachix December 11, 2022 18:29 — with GitHub Actions Inactive
@har7an
Copy link
Contributor Author

har7an commented Dec 11, 2022

@imsnif Here's my xtask proposal PR. It's not entirely done yet (see description above). If you like, you can try and test it: It's already capable of most things (I think) we need. The available commands can be found with cargo xtask --help. Also, unlike cargo make, you can call it from anywhere in the repository. ;)

If you think there's something missing I haven't covered above, please let me know!

Curious to hear if you spot the "bonus" I added in terms of telling which command/task is currently being run...

@imsnif
Copy link
Member

imsnif commented Dec 12, 2022

@har7an - I've got a lot on my plate atm. Would be happy to give some specific feedback if you have any questions or are unsure of something? Otherwise I'd rather give it all a thorough look when you're positive it's good, finished and does everything it needs to.

@har7an har7an force-pushed the feature/cargo-xtask branch from bb8b168 to deb0921 Compare December 14, 2022 13:12
@har7an har7an temporarily deployed to cachix December 14, 2022 13:12 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 15, 2022 07:11 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 15, 2022 07:25 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 15, 2022 07:28 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 15, 2022 07:31 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 15, 2022 07:35 — with GitHub Actions Inactive
@har7an
Copy link
Contributor Author

har7an commented Dec 15, 2022

@imsnif It's done now. As far as I can tell it has full feature-parity with previous cargo make. I even added the publish pipeline already with some added features/benefits. I hope you like it and I'm looking forward to your feedback!

@har7an har7an temporarily deployed to cachix December 15, 2022 07:37 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Dec 15, 2022

Hey @har7an - this already looks great! Looking forward to using this. A few comments:

  1. It's important that we be able to pass arguments to commands. Eg. I should be able to do cargo xtask run --layout /path/to/layout/file.kdl or even cargo xtask run -- --layout /path/to/layout/file.kdl
  2. Would be cool if we have some sort of "migration document" that people can use to quickly find out how to move from the old commands to the new ones. Better yet, alias all the old commands to error messages that will explain the migration and tell the user exactly what to do: install xtask <link>, run the following command: cargo xtask <your stuff here>
  3. With cargo make I've always had the problem that cargo make run is super slow. Even when there's no change to any crate, it still goes through all the crates one by one, tries to compile them and then (for some reason) sleeps for almost 2 seconds before moving to the next crate. cargo xtask run does the exact same thing. With the plugins it can't be helped, but I wonder why this happens with all the workspace crates... In any case, if I'm not working on the plugins I usually do cargo run -- --data-dir target/dev-data - would be cool if we had this as an xtask task... maybe cargo xtask quickrun?

@har7an har7an temporarily deployed to cachix December 15, 2022 09:49 — with GitHub Actions Inactive
@har7an
Copy link
Contributor Author

har7an commented Dec 15, 2022

  1. It's important that we be able to pass arguments to commands.

Good catch, I forgot a -- in the code. Thanks!

2. Would be cool if we have some sort of "migration document" that people can use to quickly find out how to move from the old commands to the new ones.

Have you tried running cargo make with this build?

3. would be cool if we had this as an xtask task...

I see. I added a --data-dir argument to xtask run, which, when present, will run zellij with the disable_automatic_asset_installation feature and pass --data-dir to the binary being run, too. Is that what you had in mind?

Oh btw, great "feature" about xtask (although irrelevant for you): #1929 doesn't occur any longer. :)

xtask is a cargo alias that is used to extend the cargo build system
with custom commands. For an introduction to xtask, see here:
https://github.com/matklad/cargo-xtask/

The idea is that instead of writing makefiles, xtask requires no
additional dependencies except `cargo` and `rustc`, which must be
available to build the project anyway.

This commit provides a basic implementation of the `build` and `test`
subcommands.
to perform different useful tasks. Includes:

- clippy
- format
- "make" (composite)
- "install" (composite)

Also add more options to `build` to selectively compile plugins or leave
them out entirely.
- `wasm_opt_plugins` and
- `manpage`

that perform other build commands. Add thorough documentation on what
each of these does and also handle the new `build` cli flags
appropriately.
that perform multiple atomic xtask commands sequentially in a pipeline
sort of fashion.
and add documentation.
which performs an 'install' and copies the resulting zellij binary along
with some other assets to a `target/dist` folder.
which will allow very quick iterations when not changing the plugins
between builds.
@har7an har7an force-pushed the feature/cargo-xtask branch from 52ed2e1 to a588bb7 Compare December 15, 2022 11:31
@har7an har7an temporarily deployed to cachix December 15, 2022 11:31 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 16, 2022 08:21 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 16, 2022 11:10 — with GitHub Actions Inactive
@har7an har7an force-pushed the feature/cargo-xtask branch from 74da1a5 to 9dd4c1a Compare December 16, 2022 11:22
@har7an har7an temporarily deployed to cachix December 16, 2022 11:22 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 16, 2022 16:37 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 16, 2022 16:39 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 16, 2022 16:43 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 17, 2022 09:26 — with GitHub Actions Inactive
because the zellij binary needs to include the plugins.
@har7an har7an temporarily deployed to cachix December 17, 2022 09:30 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 17, 2022 09:37 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 17, 2022 09:39 — with GitHub Actions Inactive
@har7an har7an force-pushed the feature/cargo-xtask branch from d6acb24 to 7eec160 Compare December 17, 2022 10:56
@har7an har7an temporarily deployed to cachix December 17, 2022 10:57 — with GitHub Actions Inactive
@har7an har7an temporarily deployed to cachix December 17, 2022 12:41 — with GitHub Actions Inactive
@har7an har7an merged commit d1f5015 into zellij-org:main Dec 17, 2022
@har7an har7an deleted the feature/cargo-xtask branch August 28, 2023 17:30
har7an added a commit to har7an/zellij that referenced this pull request Jan 19, 2025
which really should have been deleted as part of zellij-org#2012. This hasn't been
updated for more than 2 years now and I don't expect anyone to still use
this. Our build process is now managed by `cargo xtask`.
har7an added a commit that referenced this pull request Jan 25, 2025
* chore: Remove deprecated `Makefile.toml`

which really should have been deleted as part of #2012. This hasn't been
updated for more than 2 years now and I don't expect anyone to still use
this. Our build process is now managed by `cargo xtask`.

* Cargo: Update the Rust toolchain to 1.84.0

from 1.75.0 which has been deprecated for a while now. Along with this
change, the `wasm32-wasi` target is no longer available (see subsequent
commit for additional info).

* chore: Rename `wasm32-wasi` to `wasm32-wasip1`

as required by the Rust project. The `wasm32-wasi` target name has been
retired and will likely be reused at a later time, although to express
an entirely different target (i.e. implementation of the WASI standard).

For additional information, see:

  - https://blog.rust-lang.org/2024/04/09/updates-to-rusts-wasi-targets.html
  - https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#wasi-01-target-naming-changed

* chore: Drop `rust-analysis` component

from the `rust-toolchain.toml` definition. This was added way back in
2021 via 8688569, and while I'm not sure what it expressed back then,
nowadays it refers to [Metadata for RLS][1], which apparently was an
early language server implementation and has long since been replaced by
*rust-analyzer*.

We don't want to propose or enforce the use of a specific toolchain and
in any case, setting this up properly is the job of a developers
IDE/Editor.

[1]:
https://github.com/rust-lang/rustup/blob/1f06e3b31d444f3649dd51225a9d38362f7313e0/doc/user-guide/src/concepts/components.md#previous-components

* chore: Adhere to type rename

from `std::panic::PanicInfo` to `std::panic::PanicHookInfo`, which was
introduced in Rust 1.81.0. For additional information, see:

- https://releases.rs/docs/1.81.0/#compatibility-notes
- rust-lang/rust#115974

* fix(utils/data): Adhere to expected case

in match arm patterns, since the expression being matched against has
been modified using `to_ascii_lowercase`. Hence, we cannot have upper
case ASCII chars in the expressions (these arms were previously no-ops).

* fix(utils): Derive `Hash` manually

in `input/layout` since the `PartialEq` trait is also implemented
manually. Previously the `Hash` impl wasn't consistent with the `Eq`
impl, which can have weird effects when using these types in e.g.
`HashMap`s or similar types. For additional information, see:

  - https://rust-lang.github.io/rust-clippy/master/index.html#derived_hash_with_manual_eq
  - https://doc.rust-lang.org/stable/std/hash/trait.Hash.html#hash-and-eq

* fix(utils): Derive `Hash` manually

in `pane_size` since the `PartialEq` trait is also implemented manually.
Previously the `Hash` impl wasn't consistent with the `Eq` impl, which
can have weird effects when using these types in e.g. `HashMap`s or
similar types. For additional information, see:

  - https://rust-lang.github.io/rust-clippy/master/index.html#derived_hash_with_manual_eq
  - https://doc.rust-lang.org/stable/std/hash/trait.Hash.html#hash-and-eq

* fix(server): Don't redeclare variables

with their same names. Latest rust toolchains reject this code.

* chore(actions): Use non-archived toolchain setup

for the Rust toolchain. The previously used action has been archived
over a year ago. The new one should also support reading our
`rust-toolchain.toml`, so we no longer have to keep track of the
toolchain in multiple places.

* chore(actions): Add some space to YAML files

to make them better visually parsable.

* ci: Remove toolchain update Job

since as far as I can tell, this isn't used any more.

* ci: Fix invalid actions specification

and only request an action without running other code.

* CHANGELOG: Add PR #3945.
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.

2 participants