Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

fetch-binary-deps.py should place files in non-build folder #117

Open
mossheim opened this issue May 11, 2020 · 5 comments
Open

fetch-binary-deps.py should place files in non-build folder #117

mossheim opened this issue May 11, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@mossheim
Copy link
Contributor

Currently, running fetch-binary-deps.py downloads and unpacks large files within
the build/ folder. This is undesirable for a couple of reasons:

  • it makes it difficult (perhaps impossible?) to clear the build directory and start with a fresh build without re-running this script, which takes several minutes on my connection to download all the files
  • it makes it difficult (perhaps impossible?) to have multiple build directories

i'm not very familiar with this project, so i'm not sure if the intent is to have a separate set of binary dependencies per build, but it would be nice to at least not have this destination hardcoded, because it limits some of the natural flexibility that cmake affords. it would also be nice to have the option to use the same binary dependencies for multiple builds, since they take quite a while to download.

it seems like most of what this script is doing could also be done in CMake; it has easy utilities for downloading files, untarring, and checking sha sums. i'd wager the resulting code would be even smaller than this python script. have you considered that?/would you want a PR for that?

@lnihlen
Copy link
Member

lnihlen commented May 11, 2020

Fine to put them in a different spot, although the CMake is hard-coded right now to expect the unpacked downloads in build/install-ext, and that would be a little harder to change.

Also fine to do it in CMake. I expect the curve ball will be verifying the hashes. I would gladly accept a PR for one or both changes.

Thanks!

@mossheim
Copy link
Contributor Author

ok! just to be clear, is your ideal setup just one set of binary dependencies for the entire project? or one per build?

I expect the curve ball will be verifying the hashes.

i think you'll be pleasantly surprised :)

@lnihlen
Copy link
Member

lnihlen commented May 11, 2020

So what would the differences between set-per-build and set-per-project look like? I had been thinking those scenarios were kind of the same. But recently I noticed I'll have to rebuild all the deps on Windows in Debug mode if I want to fix the windows debug build.. Is that what your talking about?

@mossheim
Copy link
Contributor Author

well there are at least 3 levels of granularity here -- where X is some dependency, you can:

  1. use a single copy of X for the entire project
  2. use a single copy of X for single build (i.e., a single folder in which you run cmake <options>)
  3. use a single copy of X for single build+configuration (i.e. what you're seeing where deps are built separately for each build configuration, like Debug and Release)

if i understand correctly that what this script does is download prebuilt executables and/or libraries and unpack them to a predictable location, then you may just want (1), as that saves time and storage. i think general cmake approach is to favor (2) and (3), but that seems like overkill to me here. i haven't thought about it too much...

@lnihlen
Copy link
Member

lnihlen commented May 11, 2020

Ah thanks for explaining! I prefer option (1) here. I don't want there to be more than a very few (hopefully 1 for users) different build configurations. The two that exist right now are the release build and a special build "Coverage" to run the llvm-cov tools and collect the code coverage data for aggregation.

@lnihlen lnihlen added this to To Do in Infrastructure Workstream via automation Jul 29, 2020
@lnihlen lnihlen added the enhancement New feature or request label Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

2 participants