kornelski / cargo-deb

Make Debian packages directly from Rust/Cargo projects
https://lib.rs/cargo-deb
MIT License
431 stars 52 forks source link

README.md file exists, but is not specified in `readme` Cargo.toml field #20

Closed EdJoPaTo closed 1 week ago

EdJoPaTo commented 2 years ago

The cargo reference states:

If no value is specified for this field, and a file named README.md, README.txt or README exists in the package root, then the name of that file will be used.

cargo-deb should not produce a warning for this and follow cargos behaviour of assuming one of these files. (Especially as the warning knows that the file exists)

John15321 commented 2 years ago

image From the looks of it it is specified

I run the packaging stage just now and I don't see any errors, If Im correct we should close this issue @EdJoPaTo

$ cargo publish --dry-run                                                                                                                                                                                                                                
    Updating crates.io index
   Packaging cargo-deb v1.38.1 (/Users/john/Documents/Programming/cargo-deb)
   Verifying cargo-deb v1.38.1 (/Users/john/Documents/Programming/cargo-deb)
  Downloaded xz2 v0.1.7
  Downloaded crc v2.1.0
  Downloaded ar v0.9.0
  Downloaded zopfli v0.6.0
  Downloaded cargo_toml v0.11.5
  Downloaded typed-arena v2.0.1
  Downloaded md5 v0.7.0
  Downloaded crc-catalog v1.1.1
  Downloaded quick-error v2.0.1
  Downloaded iter-read v0.3.1
  Downloaded lzma-sys v0.1.19
  Downloaded 11 crates (905.3 KB) in 0.68s
   Compiling libc v0.2.126
   Compiling cfg-if v1.0.0
   Compiling autocfg v1.1.0
   Compiling proc-macro2 v1.0.39
   Compiling unicode-ident v1.0.0
   Compiling syn v1.0.96
   Compiling crossbeam-utils v0.8.8
   Compiling serde_derive v1.0.137
   Compiling lazy_static v1.4.0
   Compiling serde v1.0.137
   Compiling memchr v2.5.0
   Compiling scopeguard v1.1.0
   Compiling cc v1.0.73
   Compiling rayon-core v1.9.3
   Compiling pkg-config v0.3.25
   Compiling log v0.4.17
   Compiling either v1.6.1
   Compiling serde_json v1.0.81
   Compiling regex-syntax v0.6.26
   Compiling crc-catalog v1.1.1
   Compiling termcolor v1.1.3
   Compiling ryu v1.0.10
   Compiling unicode-width v0.1.9
   Compiling typed-arena v2.0.1
   Compiling remove_dir_all v0.5.3
   Compiling fastrand v1.7.0
   Compiling humantime v2.1.0
   Compiling cargo-deb v1.38.1 (/Users/john/Documents/Programming/cargo-deb/target/package/cargo-deb-1.38.1)
   Compiling itoa v1.0.2
   Compiling iter-read v0.3.1
   Compiling byteorder v1.4.3
   Compiling adler32 v1.2.0
   Compiling glob v0.3.0
   Compiling ar v0.9.0
   Compiling quick-error v2.0.1
   Compiling md5 v0.7.0
   Compiling memoffset v0.6.5
   Compiling crossbeam-epoch v0.9.8
   Compiling rayon v1.5.3
   Compiling crc v2.1.0
   Compiling itertools v0.10.3
   Compiling getopts v0.2.21
   Compiling lzma-sys v0.1.19
   Compiling zopfli v0.6.0
   Compiling crossbeam-channel v0.5.4
   Compiling num_cpus v1.13.1
   Compiling atty v0.2.14
   Compiling filetime v0.2.16
   Compiling xattr v0.2.3
   Compiling tempfile v3.3.0
   Compiling quote v1.0.18
   Compiling aho-corasick v0.7.18
   Compiling tar v0.4.38
   Compiling regex v1.5.6
   Compiling xz2 v0.1.7
   Compiling crossbeam-deque v0.8.1
   Compiling env_logger v0.9.0
   Compiling toml v0.5.9
   Compiling cargo_toml v0.11.5
    Finished dev [unoptimized + debuginfo] target(s) in 13.04s
   Uploading cargo-deb v1.38.1 (/Users/john/Documents/Programming/cargo-deb)
warning: aborting upload due to dry run
EdJoPaTo commented 2 years ago

Ah, it seems like a misunderstanding.

I use cargo deb in projects of mine which do not explicitly state readme in their Cargo.toml. cargo deb then creates a warning about that which I think is weird. For example this Github Actions run also logs the warning: https://github.com/EdJoPaTo/mqttui/runs/6709975364?check_suite_focus=true#step:11:9

John15321 commented 2 years ago

Ah, it seems like a misunderstanding.

I use cargo deb in projects of mine which do not explicitly state readme in their Cargo.toml. cargo deb then creates a warning about that which I think is weird. For example this Github Actions run also logs the warning: https://github.com/EdJoPaTo/mqttui/runs/6709975364?check_suite_focus=true#step:11:9

From what I can see you do not have your readme explicitly stated in your Cargo.toml. And cargo-deb is right to warn your about that as that's what cargo itself does when you publish something, you can check it by running:

cargo publish --dry-run

On your repo, this should also warn you and in this case cargo Deb just passes the warning forward

John15321 commented 2 years ago

By publish i mean any form of creating packaging also

EdJoPaTo commented 2 years ago

Interesting. I havn't thought about checking commands like cargo publish about similar behaviour. I only checked the clippy lints for cargo (cargo clippy -- -W clippy::cargo) which also do not care about the missing readme entry, only for missing keywords / categories.

I just tested them, but neither cargo publish --dry-run or cargo package do seem to print a warning there while cargo deb does.

John15321 commented 2 years ago

Interesting. I havn't thought about checking commands like cargo publish about similar behaviour. I only checked the clippy lints for cargo (cargo clippy -- -W clippy::cargo) which also do not care about the missing readme entry, only for missing keywords / categories.

I just tested them, but neither cargo publish --dry-run or cargo package do seem to print a warning there while cargo deb does. @EdJoPaTo

Hmm weird, it should. It does in my projects, that's why I know about it because I encountered this warning recently. But anyway if your add:

readme = "README.md"

in your [package] sections it should be good, as you can see here after I added it locally the warning is gone:

image

EdJoPaTo commented 2 years ago

Personally I like using sane defaults which don't require to specify everything explicitly. The Cargo manifest states that default which is totally fine to me.

So yeah, I could state that explicitly to get rid of the warning but that should result in the exact same behaviour while having a more bloated config. I think its better to not have that warning in the first place as its the default already. That would make a simple tool a bit better to use.

EdJoPaTo commented 2 years ago

From your screenshot / Home directory path in your first answer I assume you are using macOS. I just ran cargo package and cargo publish --dry-run on my MacBook which also does not warn about it. (My main system is a Linux based system). It would be interesting in which cases that warning is shown then also to find the reasoning behind it. I also checked nightly and 1.57 without luck. Not sure where it might originate then.

John15321 commented 2 years ago

Hmm wait im actualy wrong here i just tested it on that package of mine I mentioned and it does print this error:

image

But that's was when I didn't have the repository key in my package configuration. Having the readme doesn't seem to change a thing. But now its a matter of, hey maybe its the debian package that requires having a readme as a firm of basic documentation? If so then shouldn't the build actually even fail if the readme is not specified directly?

EdJoPaTo commented 2 years ago

I think it might be a good idea to fail if there is no readme at all. There should be a --allow-missing-readme or something like that like there is --allow-dirty for example.

If there is a readme which can be inferred from the default its better to use that in my opinion as cargo does the same there too. For example checking cargo read-manifest | jq also shows README.md for readme in my case so Cargo is aware of it already.

John15321 commented 2 years ago

I think it might be a good idea to fail if there is no readme at all. There should be a --allow-missing-readme or something like that like there is --allow-dirty for example.

If there is a readme which can be inferred from the default its better to use that in my opinion as cargo does the same there too. For example checking cargo read-manifest | jq also shows README.md for readme in my case so Cargo is aware of it already.

Yes, cargo infers the default readme:

https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata

John15321 commented 2 years ago

@EdJoPaTo whats the jq command btw?

John15321 commented 2 years ago

I am not a maintainer here although Im happy to help

@kornelski could you take a look at this issue? If the readme is required in some way to create a .deb package it should indeed throw an error if we are not certain of the readme file

EdJoPaTo commented 2 years ago

@EdJoPaTo whats the jq command btw?

Its a command line helper to interact with json data. See https://stedolan.github.io/jq/

John15321 commented 2 years ago

NIce, thanks :)

John15321 commented 2 years ago

@EdJoPaTo btw about your project. I am a control engineering and robotics student and had my fair share of industrial IoT and your project looks really cool. What use cases does it have? Im a little shaky on IoT protocols

EdJoPaTo commented 2 years ago

mqttui is for viewing MQTT messages on a broker. For example when testing around with stuff to see whats actually happening.

But these messages should be marked off-topic in this issue as they are not about the readme option :innocent:

dcampbell24 commented 1 month ago

cargo deb currently does not produce any warnings when the readme is not specified and it reads README.md into the extended-description. I think this issue can be closed.