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

Support non binary files inside rust target directory #3

Open
danyspin97 opened this issue Jun 4, 2022 · 9 comments
Open

Support non binary files inside rust target directory #3

danyspin97 opened this issue Jun 4, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@danyspin97
Copy link
Owner

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.

@danyspin97 danyspin97 added this to the rinstall 0.2.1 milestone Jun 4, 2022
@danyspin97 danyspin97 added the enhancement New feature or request label Jun 4, 2022
@danyspin97 danyspin97 reopened this Nov 14, 2022
@classabbyamp
Copy link
Contributor

classabbyamp commented Mar 25, 2023

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
Copy link
Owner Author

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
Copy link
Contributor

However I had some problems with cargo directory structure

what kind of problems?

@danyspin97
Copy link
Owner Author

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

rust-lang/cargo#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
Copy link
Contributor

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Contributor

seems reasonable to me 👍

@Firstyear
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants