Skip to content

prost: Move external tool into build-script #831

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

Closed
wants to merge 2 commits into from

Conversation

NickeZ
Copy link
Collaborator

@NickeZ NickeZ commented Nov 12, 2021

No description provided.

@benma
Copy link
Collaborator

benma commented Nov 14, 2021

I have to say the 4000k+ new files in the vendor folder hurt! I think the previous PR #829 that moved prost-build into the Dockerfile was a better solution because of this. And as that PR states, it should speed up builds, right? 🤔

What happens if you disable the default features in the prost-build dep? Would that reduce the number of vendored files?

@NickeZ
Copy link
Collaborator Author

NickeZ commented Nov 15, 2021

Good idea to try reducing features, yeah the other option would be to install the tool in the container instead.

If we keep it as it is it complicates cargo test, so I suggest we do one of the solutions.

@NickeZ
Copy link
Collaborator Author

NickeZ commented Nov 15, 2021

The thing is, cargo vendor also eventually removes dependencies that are removed from your dependency tree. So I'm not that scared by vendoring deps that are "necessary". There is no risk of a lot of unused code.

In the end this also makes the project more self-reliant. If we build it in a dockerfile and crates.io has downtime / or some government takes it down, then we cannot rebuild the docker image / project anymore...

@benma
Copy link
Collaborator

benma commented Nov 15, 2021

The thing is, cargo vendor also eventually removes dependencies that are removed from your dependency tree. So I'm not that scared by vendoring deps that are "necessary". There is no risk of a lot of unused code.

Yeah that is true, but still ;)

In the end this also makes the project more self-reliant. If we build it in a dockerfile and crates.io has downtime / or some government takes it down, then we cannot rebuild the docker image / project anymore...

I think this is a lesser issue - Dockerfiles are built very rarely, so temporary downtimes are not so bad. We also generally expect Docker builds to become non-reproducible over time or even fail to build, as inevitably some upstream stuff changes or disappears.

If we keep it as it is it complicates cargo test, so I suggest we do one of the solutions.

One of, being this PR or moving it to the Dockerfile?

@NickeZ
Copy link
Collaborator Author

NickeZ commented Nov 16, 2021

One of, being this PR or moving it to the Dockerfile?

Yeah exactly

@x1ddos
Copy link
Contributor

x1ddos commented Nov 19, 2021

So I'm not that scared by vendoring deps that are "necessary". There is no risk of a lot of unused code.

i am scared and i think there's a substantial risk:

@benma
Copy link
Collaborator

benma commented Nov 19, 2021

Let's go with #829 instead. The Docker image is already uploaded with tag 22.

@benma benma closed this Nov 19, 2021
@benma
Copy link
Collaborator

benma commented Nov 19, 2021

So I'm not that scared by vendoring deps that are "necessary". There is no risk of a lot of unused code.

i am scared and i think there's a substantial risk:

* https://kerkour.com/rust-crate-backdoor/

* https://codeandbitters.com/published-crate-analysis/

The files are either in vendor, or built into prost-build (which does not vendor, so fetched upstream every time). Is that a significant difference in terms of supply chain risk? I would not think it is a big difference, either can do harm. Just because they are not vendored it does not mean they don't exist.

The solution here is to maybe patch the prost dep to have less transitive deps or disable them somehow (if not already possible via feature gates).

@x1ddos
Copy link
Contributor

x1ddos commented Nov 19, 2021

right. the deps do exist either way but look at the insane deps graph:

prost-build v0.1.0
├── clap v3.0.0-beta.1
│   ├── atty v0.2.14
│   │   └── libc v0.2.71
│   ├── bitflags v1.2.1
│   ├── clap_derive v3.0.0-beta.1 (proc-macro)
│   │   ├── heck v0.3.1
│   │   │   └── unicode-segmentation v1.6.0
│   │   ├── proc-macro-error v0.4.12
│   │   │   ├── proc-macro-error-attr v0.4.12 (proc-macro)
│   │   │   │   ├── proc-macro2 v1.0.18
│   │   │   │   │   └── unicode-xid v0.2.1
│   │   │   │   ├── quote v1.0.7
│   │   │   │   │   └── proc-macro2 v1.0.18 (*)
│   │   │   │   ├── syn v1.0.33
│   │   │   │   │   ├── proc-macro2 v1.0.18 (*)
│   │   │   │   │   ├── quote v1.0.7 (*)
│   │   │   │   │   └── unicode-xid v0.2.1
│   │   │   │   └── syn-mid v0.5.0
│   │   │   │       ├── proc-macro2 v1.0.18 (*)
│   │   │   │       ├── quote v1.0.7 (*)
│   │   │   │       └── syn v1.0.33 (*)
│   │   │   │   [build-dependencies]
│   │   │   │   └── version_check v0.9.2
│   │   │   ├── proc-macro2 v1.0.18 (*)
│   │   │   ├── quote v1.0.7 (*)
│   │   │   └── syn v1.0.33 (*)
│   │   │   [build-dependencies]
│   │   │   └── version_check v0.9.2
│   │   ├── proc-macro2 v1.0.18 (*)
│   │   ├── quote v1.0.7 (*)
│   │   └── syn v1.0.33 (*)
│   ├── indexmap v1.4.0
│   │   [build-dependencies]
│   │   └── autocfg v1.0.0
│   ├── lazy_static v1.4.0
│   ├── os_str_bytes v2.3.1
│   ├── strsim v0.10.0
│   ├── termcolor v1.1.0
│   ├── textwrap v0.11.0
│   │   └── unicode-width v0.1.8
│   ├── unicode-width v0.1.8
│   └── vec_map v0.8.2
└── prost-build v0.7.0
    ├── bytes v1.0.0
    ├── heck v0.3.1 (*)
    ├── itertools v0.9.0
    │   └── either v1.5.3
    ├── log v0.4.8
    │   └── cfg-if v0.1.10
    ├── multimap v0.8.1
    ├── petgraph v0.5.1
    │   ├── fixedbitset v0.2.0
    │   └── indexmap v1.4.0 (*)
    ├── prost v0.7.0
    │   ├── bytes v1.0.0
    │   └── prost-derive v0.7.0 (proc-macro)
    │       ├── anyhow v1.0.31
    │       ├── itertools v0.9.0 (*)
    │       ├── proc-macro2 v1.0.18 (*)
    │       ├── quote v1.0.7 (*)
    │       └── syn v1.0.33 (*)
    ├── prost-types v0.7.0
    │   ├── bytes v1.0.0
    │   └── prost v0.7.0 (*)
    └── tempfile v3.1.0
        ├── cfg-if v0.1.10
        ├── libc v0.2.71
        ├── rand v0.7.3
        │   ├── getrandom v0.1.14
        │   │   ├── cfg-if v0.1.10
        │   │   └── libc v0.2.71
        │   ├── libc v0.2.71
        │   ├── rand_chacha v0.2.2
        │   │   ├── ppv-lite86 v0.2.8
        │   │   └── rand_core v0.5.1
        │   │       └── getrandom v0.1.14 (*)
        │   └── rand_core v0.5.1 (*)
        └── remove_dir_all v0.5.3
    [build-dependencies]
    └── which v4.0.2
        ├── libc v0.2.71
        └── thiserror v1.0.21
            └── thiserror-impl v1.0.21 (proc-macro)
                ├── proc-macro2 v1.0.18 (*)
                ├── quote v1.0.7 (*)
                └── syn v1.0.33 (*)

half of this comes from clap, just for 2 cmd line args - could be easily replaced with a std::env::args.

tempfile pulls in the whole libc, rand_chacha and other stuff. that's just crazy.

are there alternatives to prost-build?

@NickeZ
Copy link
Collaborator Author

NickeZ commented Nov 20, 2021

I made a PR to remove clap in favour of lexopt that doesn't have any deps and has the same level of correctness. But that wasn't deemed necessary when we go to prebuild the tool in docker

@NickeZ
Copy link
Collaborator Author

NickeZ commented Nov 20, 2021

I think prost build the lib has a reasonable amount of deps and for the right purposes. Many items in that tree are duplicates

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.

3 participants