danyspin97 / rinstall

Declarative install for programs
GNU General Public License v3.0
28 stars 2 forks source link

Support non binary files inside rust target directory #3

Open danyspin97 opened 2 years ago

danyspin97 commented 2 years ago

kanidm is correctly putting completions inside the target directory (OUT_DIR), as build.rs shouldn't modify any file outside this directory:

   completions:
      bash:
        - target/release/build/completions/kanidmd.bash
        - target/release/build/completions/kanidm_badlist_preprocess.bash
        - target/release/build/completions/kanidm.bash

rinstall should automatically search for this files inside the OUT_DIR. I think it could never happen that the same file is inside both the project target directory and the OUT_DIR.

classabbyamp commented 1 year ago

what about having something like this?

   completions:
      bash:
        - $OUT_DIR/completions/kanidmd.bash
        - $OUT_DIR/completions/kanidm_badlist_preprocess.bash
        - $OUT_DIR/completions/kanidm.bash

I don't think this env var would be available directly for use, but it should be derivable similar to how the exes are found

edit: or maybe @OUT_DIR@ instead of $OUT_DIR

danyspin97 commented 1 year ago

This is one of the two ideas that I have considered doing. I actually went the other way around, taken from the changelog:

For rust packages, all the files will be searched inside of the output directory (usually target) and, if they don't exist, inside the project directory. This allows rinstall to correctly get files generated by build.rs without adding too much clutter in the install.yml. To force rinstall to get a file in the project directory, use the $PROJECTDIR placeholder.

However I had some problems with cargo directory structure and I wasn't able to made upstream change their mind, this is way the development of rinstall have been slowed down in the last months.

While OUT_DIR is a good solution, the disadvantage is that the install.yml becomes cluttered, especially in huge projects like kanidm. With the repetition of $OUT_DIR throughout all the install.yml I thought that it would be harder to advocate in favor of rinstall (which I admit is also a big part of

classabbyamp commented 1 year ago

However I had some problems with cargo directory structure

what kind of problems?

danyspin97 commented 1 year ago

The issues have been described here by kanidm developer (@Firstyear):

https://github.com/rust-lang/cargo/issues/9858

I have also opened a thread in the official Rust Zulip instance:

https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/New.20.60GEN_DIR.60.20var.20for.20generated.20files.20in.20build.2Ers/near/302814870

classabbyamp commented 1 year ago

ok wow, that sounds impossible to work around on rinstall's side.

what about going through $cratename-*/ and finding the one worth the newest mtime? kinda ugly and hacky, but I think it would work.

another option might be to require printing the value of OUT_DIR in build.rs but that seems bad to require devs to do something

danyspin97 commented 1 year ago

what about going through $cratename-*/ and finding the one worth the newest mtime? kinda ugly and hacky, but I think it would work.

The issue here is that there could be multiple crates and we don't know their names.

another option might be to require printing the value of OUT_DIR in build.rs but that seems bad to require devs to do something

Agreed, advocating in favor of rinstall would be harder with this.

danyspin97 commented 12 months ago

This is the current setting of outdir in build.rs for rinstall:

    let outdir = PathBuf::from(std::env::var("OUT_DIR").unwrap())
        .ancestors()
        .nth(3)
        .unwrap()
        .to_path_buf();

And this is the current install.yml file for rinstall:

rinstall: 0.2.0
pkgs:
  rinstall:
    type: rust
    exe:
      - rinstall
    docs:
      - README.md
    licenses:
      - LICENSE.md
    man:
      - man/rinstall.1
    completions:
      bash:
        - completions/rinstall.bash
      elvish:
        - completions/rinstall.elv
      fish:
        - completions/rinstall.fish
      zsh:
        - completions/_rinstall

With this configuration, the generated files (completions and man page) are correctly handled by rinstall:

> ./target/x86_64-unknown-linux-musl/release/rinstall install --rust-target-triple x86_64-unknown-linux-musl --system
>>> Package rinstall
Would install target/x86_64-unknown-linux-musl/release/rinstall -> /usr/local/bin/rinstall
Would install target/x86_64-unknown-linux-musl/release/man/rinstall.1 -> /usr/local/share/man/man1/rinstall.1
Would install README.md -> /usr/local/share/doc/rinstall/README.md
Would install target/x86_64-unknown-linux-musl/release/completions/rinstall.bash -> /usr/local/share/bash-completion/completions/rinstall.bash
Would install target/x86_64-unknown-linux-musl/release/completions/rinstall.elv -> /usr/local/share/elvish/lib/rinstall.elv
Would install target/x86_64-unknown-linux-musl/release/completions/rinstall.fish -> /usr/local/share/fish/vendor_completions.d/rinstall.fish
Would install target/x86_64-unknown-linux-musl/release/completions/_rinstall -> /usr/local/share/zsh/site-functions/_rinstall
Would install LICENSE.md -> /usr/local/share/licenses/rinstall/LICENSE.md
Would install pkginfo -> /usr/local/var/rinstall/rinstall.pkg

One possible disadvantage is that there could be files or directories with the same name as the the generated files. Since rinstall allows to rename later, temporary names can be used in the build.rs file, so I don't think this could be an issue.

I will try again with kanidm, and if it works fine I'd propose to close this issue as fixed. Let me know your thoughts, I am sure that there is something I am missing.

classabbyamp commented 11 months ago

seems reasonable to me :+1:

Firstyear commented 11 months ago

Yeah, cargo really isn't setup to output files for anything 3rd party like shell completions. Which is a bit annoying since cargo is very baked-into rust.