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

cargo install fj-app has been broken since version 0.24.0 #1356

Closed
hannobraun opened this issue Nov 15, 2022 · 10 comments · Fixed by #1364 or #1373
Closed

cargo install fj-app has been broken since version 0.24.0 #1356

hannobraun opened this issue Nov 15, 2022 · 10 comments · Fixed by #1364 or #1373
Assignees
Labels
topic: build Anything relating to the build system. type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

Here's what happens, as of now (the latest version being 0.24.0), when running cargo install fj-app:

error: failed to run custom build command for `fj-app v0.24.0`

Caused by:
  process didn't exit successfully: `/tmp/cargo-installoNLrtf/release/build/fj-app-b6379994194ca0ad/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: WithPath { path: "/home/hanno/.cargo/registry/src/models/star", err: Io(Custom { kind: NotFound, error: Error { depth: 0, inner: Io { path: Some("/home/hanno/.cargo/registry/src/models/star"), err: Os { code: 2, kind: NotFound, message: "No such file or directory" } } } }) }', /home/hanno/.cargo/registry/src/github.com-1ecc6299db9ec823/fj-app-0.24.0/build.rs:31:26
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This was most likely introduced by #1344.

@hannobraun hannobraun added type: bug Something isn't working topic: build Anything relating to the build system. labels Nov 15, 2022
@hannobraun
Copy link
Owner Author

So, looking at the crate contents of fj-app, the package (obviously!) just contains the contents of the fj-app/ directory, while the models/ directory is in the repository root. So this should be relatively straight-forward to fix, but I'm not sure what the best way is. Add a symbolic link to models/? Or maybe add a dedicated template model to the fj-app directory?

@MartinKavik
Copy link
Contributor

Or maybe add a dedicated template model to the fj-app directory?

Sorry for the problem, I haven't published any crates yet so it wasn't super obvious for me.
I would add a dedicated template to the fj-app (I've already done it in MoonZoon CLI) - it could complicate your automatic tests a bit because of a different model location but on the other hand you can fully adapt the example to be used as a template and don't accidentally surprise users with such non-standard changes because the user will not run it while experimenting with examples.

@hannobraun
Copy link
Owner Author

Sorry for the problem, I haven't published any crates yet so it wasn't super obvious for me.

No need to apologize! It wasn't obvious to me either, or I would have caught it in review 😄

I would add a dedicated template to the fj-app (I've already done it in MoonZoon CLI) - it could complicate your automatic tests a bit because of a different model location but on the other hand you can fully adapt the example to be used as a template and don't accidentally surprise users with such non-standard changes because the user will not run it while experimenting with examples.

That reasoning makes sense. It would be my preference too. I don't think testing will be complicated much. Should be easy as adding the new crate to the workspace.

@hannobraun
Copy link
Owner Author

I'm looking into this now. Would like to see it fixed before the next release on Monday.

@hannobraun
Copy link
Owner Author

This is still broken with 0.25.0!

New error messages:

error: proc macro panicked
 --> /home/hanno/.cargo/registry/src/github.com-1ecc6299db9ec823/fj-app-0.25.0/src/model_crate.rs:4:30
  |
4 | static MODEL_TEMPLATE: Dir = include_dir!("$CARGO_MANIFEST_DIR/model-template");
  |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: message: "/home/hanno/.cargo/registry/src/github.com-1ecc6299db9ec823/fj-app-0.25.0/model-template" is not a directory

error: could not compile `fj-app` due to previous error
error: failed to compile `fj-app v0.25.0`, intermediate artifacts can be found at `/tmp/cargo-installV3XcUm`

I did some digging and found this:

Regardless of whether exclude or include is specified, the following files are always excluded:

  • Any sub-packages will be skipped (any subdirectory that contains a Cargo.toml file).

Source: https://doc.rust-lang.org/cargo/reference/manifest.html#the-exclude-and-include-fields

I'll take a look later!

@hannobraun hannobraun reopened this Nov 21, 2022
@hannobraun hannobraun changed the title cargo install fj-app is broken for version 0.24.0 cargo install fj-app is broken since version 0.24.0 Nov 21, 2022
@hannobraun hannobraun changed the title cargo install fj-app is broken since version 0.24.0 cargo install fj-app has been broken since version 0.24.0 Nov 21, 2022
@hannobraun
Copy link
Owner Author

Okay, so the problem is indeed that Cargo will not include any subdirectory with a Cargo.toml and will not be convinced otherwise, even if you explicitly specify the directory using include: rust-lang/cargo#6917

We could rename the file to something else to allow it to be included, then rename it again on-the-fly when creating the model. But then it won't be part of the Cargo workspace, and thus won't be part of any local or CI builds. Another option is to include the template as an archive file in the repository, but then we'd need automation to rebuild and check this archive, or it will end up broken all the time.

The bottom line is, I don't see a straight-forward solution, and I don't want to invest my time in any complicated ones right now. For the time being, I will remove the feature and re-open #1271.

Thank you for working on this, @MartinKavik! Too bad it didn't work out, for the time being.

@MartinKavik
Copy link
Contributor

Another option is to include the template as an archive file in the repository, but then we'd need automation to rebuild and check this archive, or it will end up broken all the time.

What's the problem with including that archive file in the repository? I think we just need to:

  1. Update paths based on OUT_DIR: in build.rs and in the file with include_bytes!.
  2. Add a condition to build.rs to not build the archive when there isn't the template available (it happens when cargo install is called).
  3. Check if the build.rs script is rerun on the template change (if not, we can use include or rerun-if-changed=[template_dir])

Do you see any problems with this solution / am I overlooking something? Thanks!

(I came back to this issue to think about it more because I'll have to resolve the same problem in MoonZoon sooner or later, too.)

@hannobraun
Copy link
Owner Author

What's the problem with including that archive file in the repository?

There's no inherent problem, we'd just need an automated way to make sure it's up-to-date. That seemed like more complexity than I wanted to get involved with that moment, so I opted to remove the feature instead.

I didn't think of using build.rs to update the archive though! I figured we'd need some CI-based solution, which would be more complicated.

Just to make sure we're talking about the same thing, here's the solution I envision, based on your comment:

  1. In build.rs, check if the template directory is available. If it isn't, abort.
  2. Build a .tar archive from the template directory, git commit it.
  3. include_bytes! the .tar in the code and use it to generate the model.

Is that what you had in mind? If so, I don't understand that part:

  1. Update paths based on OUT_DIR: in build.rs and in the file with include_bytes!.

We can't put the archive in OUT_DIR though, because then it won't be available when cargo installing, right?

@MartinKavik
Copy link
Contributor

Is that what you had in mind?

Exactly.

We can't put the archive in OUT_DIR though, because then it won't be available when cargo installing, right?

By "update" I meant to rewrite paths with OUT_DIR to point to a directory inside the crate that cargo is able to include even when building with cargo install. In other words to get rid off OUT_DIR. I was just looking at my old PR (https://github.com/hannobraun/Fornjot/pull/1344/files) when I was writing it so I haven't written it too clearly.

@hannobraun
Copy link
Owner Author

Ah, got it. Thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: build Anything relating to the build system. type: bug Something isn't working
Projects
None yet
2 participants