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

Un-embed fmtlib headers, instead auto-download #2439

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Dec 24, 2019

Like we do for robin-map or pybind11, auto-download the fmtlib headers
into ext. Remove embedded copies.

Disadvantages:

  • One more build step, requires a download. Maybe additional things can
    go wrong? (Mitigating: we already do this for pybind11 and robin-map,
    with no complaints that I'm aware of.)
  • This is sometimes a pain for people building behind firewalled networks
    that don't have internet access. (Mitigating: but they had to get the
    OIIO code from the net in the first place, so they could get fmt, too.)

Advantages:

  • It's just bad form to fully embed other projects if you can help it.
    Some distros have rules against it, and ASWF (if we ever go that route)
    strongly discourages it.
  • Makes it WAY easier for me to bump which version of fmt we prefer
    to use -- change one line with the tag we want, rather than updating
    several header files in their entirety.
  • Allows users to use a system install of fmtlib, or to use a
    particular/alternate version than what we had embedded.
  • Easier for us to benchmark new releases of fmtlib.

@lgritz lgritz force-pushed the lg-fmtdownload branch 2 times, most recently from 1607dc5 to bce5971 Compare January 7, 2020 21:00
@fpsunflower
Copy link
Contributor

LGTM

I see you are still copying the headers as part of the install step. Is fmtlib considered part of the public API of OIIO?

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

In some technical sense, it's not part of the public API -- the public API is OIIO::format, OIIO::sprintf, OIIO::printf, etc.

But those are variadic templates that in turn call fmt for their implementation. So I don't think they can be implemented without also making absolutely sure the fmt headers are available and included by our headers.

@fpsunflower
Copy link
Contributor

Makes sense, though it means we are effectively re-embedding it inside OIIO during install.

If an application was already using fmtlib for its own purpose, it might mean they still wind up with two copies (though one of them would be nested in the oiio include folder, so perhaps there's not too much risk for confusion ...)

I'm not really sure what the alternative is.

In any case, I think this patch is a good step.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

Yep, there will be potentially a version in oiio and a version in the app. I think because it's all templated, there's no real harm. But I agree it's ugly. I don't have an alternative at the moment.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

I just pushed an update, did some renaming of fmtlib -> fmt.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

Some day I'd like to think this will all be replace by use of std::format underneath.

@hobbes1069
Copy link
Contributor

While bundling libraries is discouraged, internet connections are not allowed on the build server (Fedora or Debian), everything the package needs should be available in the buildroot. How does this affect packaging builds?

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

Is fmt a Fedora/Debian package? If so, just install it as a dependency, no download will happen if it's already installed.

Otherwise, the only other way I can think to deal with this is as a git submodule. Is that allowed?

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

Yet another strategy is to take the download out of the cmake scripts altogether, and always assume it's a pre-installed dependency (failing the oiio build if not found), and supplying a build_fmt.bash that will download and build it for those who don't have it on their system.

@hobbes1069
Copy link
Contributor

Yup, looks like 6.2.1 is packaged. If Fedora has it I'm sure Debian has it.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

So... we are saved in this case by the fact that fmt is becoming popular enough that there evidently is a package for it. But that seems like a narrow escape to me, and I'd still like to have a principled way forward for cases where we have a dependency that is still too obscure to have standard packages on all the platforms, or where our particular need is for a version different than the prepackaged platforms (newer or older or simply desiring to lock down the version, or wanting an easy way to test new or different versions of the dependency).

There are a several potential strategies:

  1. Fully embed the package, as we do with fmt currently (prior to this PR). Some distributions disallow this, it's widely considered a poor practice (ASWF discourages it and it makes adhering to all the licenses potentially dicier).

  2. Look for it on the system and if not found (or not in the right version range), auto-download as part of our cmake config step. This is what we currently do for pybind11 and robin-map, and this PR proposes to do so for fmt. The downside is that some distros disallow packages that need internet access during the build process, and for that matter it's also a PITA for studios such as my employers where the ordinary workstations are not directly connected to the outside internet.

  3. Look for it on the system and fail if we don't find it. Require users to somehow have every dependency they need built. This places a big burden on users who don't know how to get and build the more obscure dependencies. Seems like a non-starter but I'm including it for completeness.

  4. Look for it and if not found, fail. But provide an idiot-proof script for each dependency that downloads/builds the desired version, installing in ext -- like our current src/build-scripts/build_openexr.bash. Is this also a problem for the distros that don't allow network access, or is this considered not a part of the build per se but an allowed pre-build step (much like the initial check-out of the software from the repo obviously requires internet access).

  5. Add all these dependencies as git submodules, so they get automatically checked out/downloaded at the same time and in the same step as our project, nothing needs download at build time. These projects may in fact be built (as part of our build) only if an adequate version is not found elsewhere at the system. One disadvantage to this approach is that this would make the checkout have to download a lot more material (all the submodules), most of which perhaps may not be used. Whereas approaches 2 and 4 only download what is needed.

Discussion encouraged. If at all possible, I'd like to pick one strategy and use it for all these little obscure dependencies. I'd been leaning toward moving to (2) uniformly, but I think (4) is also probably fine, and maybe I could be convinced about (5).

@fpsunflower
Copy link
Contributor

What constitutes an "obscure dependency" in this case? I think fmt and pybind11 are fairly well established at this point, so I would be fine simply listing them as dependencies (required and optional respectively).

From a package management standpoint, I think if OIIO took on policy (3) - it would simply force packages to be created for the remaining ones (like robin-map). And once the package manager supports them - maybe they would no longer qualify as "obscure" ?

@hobbes1069
Copy link
Contributor

From a packaging point of view, finding the system version is essential, but I would say an acceptable fallback is embedding with one requirement:

Fedora requires that I "rm -rf <bundled_lib>" in %prep if I'm not going to use it, this should not cause CMake to fail as long as the system version is found. So basically make the include or Cmake add_subdirectory() conditional on the system version not being found.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 8, 2020

@fpsunflower : I think fmt and pybind11 were both obscure when we first started using them, but today they are not! Robin-map is still obscure, I don't see it in homebrew or vcpkg, for example, and although there is an Ubuntu package, it's version 0.5 (we use 0.6.2, I don't know if that makes a difference) and it's only packaged for the very latest Ubuntu release, not the LTS versions used by GitHub and Travis CI.

But even these "non-obscure" packages are not without peril: I just had to bump the minimum pybind11 we accept, because #2437 (inadvertently) did a totally reasonable thing that was broken in pybind11 beyond a couple versions back. Our current use of fmt is only reliable in versions new enough to guarantee that floating point formatting be locale-independent. I think that homebrew is actually up to the latest fixed versions of both of these, but it's not clear that all platform packagers are. Until just a couple days ago, the homebrew version of OpenEXR was 2.3, so you had to build 2.4 (released several months ago) yourself if that's what you wanted (and I'm willing to bet that nobody has a packaged openexr 2.4 rpm for the CentOS 7 version we have at work -- I think the stock install of CentOS might even still be 1.x).

So I'm not so sure that we can ask users, who often struggle to get dependencies set up and build our software, to rely on strictly on standard package managers, except for the most standard and stable dependencies, like libtiff or libjpeg (I can't recall ever requiring these to be a particular recent version, literally any version of the past several years is fine).

Just from a practical point of view, I can neither support users who have a hard time finding dependency packages, nor can I be responsible for doing the packaging or updating of the several package manager recipes. I just have to provide a turnkey way for users to get and build dependencies for anything that can't absolutely be counted on to have stable and relatively recent version across all the standard package managers.

@hobbes1069 : Yes, to the best of my knowledge, for all of these, if a system version in a standard place is found, any code that looks for, downloads, builds, or uses a custom copy is disabled.

Like we do for robin-map or pybind11, auto-download the fmt headers
into ext. Remove embedded copies.

Disadvantages:

* One more build step, requires a download. Maybe additional things can
  go wrong? (Mitigating: we already do this for pybind11 and robin-map,
  with no complaints that I'm aware of.)
* This is sometimes a pain for people building behind firewalled networks
  that don't have internet access. (Mitigating: but they had to get the
  OIIO code from the net in the first place, so they could get fmt, too.)

Advantages:

* It's just bad form to fully embed other projects if you can help it.
  Some distros have rules against it, and ASWF (if we ever go that route)
  strongly discourages it.
* Makes it WAY easier for me to bump which version of fmt we prefer
  to use -- change one line with the tag we want, rather than updating
  several header files in their entirety.
* Allows users to use a system install of fmtlib, or to use a
  particular/alternate version than what we had embedded.
* Easier for us to benchmark new releases of fmtlib.
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 13, 2020

OK, my current plan is to merge this, since switching from an embedded to a downloaded fmt is still a step forward.

But longer range, I'm still mulling over removing "auto-downloads" from the config & build process. I'm leaning toward solution (4) above -- just like we provide scripts that can download and build openexr or opencolorio (for situations where somebody's system and/or package manager lacks precisely the version they need), we can do so for additional dependencies that we currently download as part of the build. This leaves it in user/builder's hands to make sure all dependencies are present, but we give them a really easy way to install what's missing.

@lgritz lgritz merged commit e3b92c8 into AcademySoftwareFoundation:master Jan 13, 2020
@lgritz lgritz deleted the lg-fmtdownload branch January 14, 2020 00:36
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 22, 2020
…n#2439)

Like we do for robin-map or pybind11, auto-download the fmt headers
into ext. Remove embedded copies.

Disadvantages:

* One more build step, requires a download. Maybe additional things can
  go wrong? (Mitigating: we already do this for pybind11 and robin-map,
  with no complaints that I'm aware of.)
* This is sometimes a pain for people building behind firewalled networks
  that don't have internet access. (Mitigating: but they had to get the
  OIIO code from the net in the first place, so they could get fmt, too.)

Advantages:

* It's just bad form to fully embed other projects if you can help it.
  Some distros have rules against it, and ASWF (if we ever go that route)
  strongly discourages it.
* Makes it WAY easier for me to bump which version of fmt we prefer
  to use -- change one line with the tag we want, rather than updating
  several header files in their entirety.
* Allows users to use a system install of fmtlib, or to use a
  particular/alternate version than what we had embedded.
* Easier for us to benchmark new releases of fmtlib.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 23, 2020
…n#2439)

Like we do for robin-map or pybind11, auto-download the fmt headers
into ext. Remove embedded copies.

Disadvantages:

* One more build step, requires a download. Maybe additional things can
  go wrong? (Mitigating: we already do this for pybind11 and robin-map,
  with no complaints that I'm aware of.)
* This is sometimes a pain for people building behind firewalled networks
  that don't have internet access. (Mitigating: but they had to get the
  OIIO code from the net in the first place, so they could get fmt, too.)

Advantages:

* It's just bad form to fully embed other projects if you can help it.
  Some distros have rules against it, and ASWF (if we ever go that route)
  strongly discourages it.
* Makes it WAY easier for me to bump which version of fmt we prefer
  to use -- change one line with the tag we want, rather than updating
  several header files in their entirety.
* Allows users to use a system install of fmtlib, or to use a
  particular/alternate version than what we had embedded.
* Easier for us to benchmark new releases of fmtlib.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Feb 15, 2020
…n#2439)

Like we do for robin-map or pybind11, auto-download the fmt headers
into ext. Remove embedded copies.

Disadvantages:

* One more build step, requires a download. Maybe additional things can
  go wrong? (Mitigating: we already do this for pybind11 and robin-map,
  with no complaints that I'm aware of.)
* This is sometimes a pain for people building behind firewalled networks
  that don't have internet access. (Mitigating: but they had to get the
  OIIO code from the net in the first place, so they could get fmt, too.)

Advantages:

* It's just bad form to fully embed other projects if you can help it.
  Some distros have rules against it, and ASWF (if we ever go that route)
  strongly discourages it.
* Makes it WAY easier for me to bump which version of fmt we prefer
  to use -- change one line with the tag we want, rather than updating
  several header files in their entirety.
* Allows users to use a system install of fmtlib, or to use a
  particular/alternate version than what we had embedded.
* Easier for us to benchmark new releases of fmtlib.

Fix BUILD_FMT_FORCE accidently left in the wrong position during debug
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.

None yet

3 participants