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

Initial rename of OpenEXR and IlmBase directories and seperation of Test #796

Merged
merged 12 commits into from
Jul 28, 2020

Conversation

oxt3479
Copy link
Contributor

@oxt3479 oxt3479 commented Jul 23, 2020

This PR effectively:

Restructures the repo's source file directory including:

Moving into a tree starting from src/
Moves all tests in these two old directories to a new separate directory tests/
Allows for building with this new configuration using CMake

PyIlmBase/ has not been touched as it could potentially be removed altogether, the only thing it contains now are pyIex and pyIexTest. This PR by default will not build PyIlmBase, but it remains as source.

After this PR:

PyIlmBase/ can be removed or relocated depending on feedback.
Cmake can be further optimized as there is some redundancy

This is a big change so if there are objections that is understandable. I want to get feedback before getting too far into things.

@meshula
Copy link
Contributor

meshula commented Jul 23, 2020

Is there a reason to move IlmImf into src in a following PR, instead of this one?

Also, since hundreds of files have moved, would you mind posting a comment showing the output of a tool like tree to show what the directory structure looks like after the reorganization, please?

@cary-ilm
Copy link
Member

cary-ilm commented Jul 23, 2020 via email

@lgritz
Copy link
Contributor

lgritz commented Jul 23, 2020

I think that IlmImf (the source to the main openexr-reading/writing library) should be in src, not tools.

In fact, I would think the usual case would be to put all source in src (one subdirectory per library or command line tool), and "tools" (at the top level) is likely to be interpreted as scripts and tools that are used part of the build, not source for things that will be part of the installed software.

@peterhillman
Copy link
Contributor

OpenColorIO has all command-line utilities in subfolders of src/apps. That seems nice and clear.

@cary-ilm
Copy link
Member

cary-ilm commented Jul 23, 2020 via email

@lgritz
Copy link
Contributor

lgritz commented Jul 24, 2020

I like Cary's layout a lot.

@lgritz
Copy link
Contributor

lgritz commented Jul 24, 2020

Will everybody throw vegetables or heavy objects at my head if I wonder out loud if we should ever switch IlmImf to OpenEXR?

@peterhillman
Copy link
Contributor

Now would be a good time to rename IlmImf, though it does provide a namespace and headers prefixed with 'Imf' not 'OpenEXR'

Pedantically, IlmImfExamples is an executable binary rather than a library, though it could also be considered a test, as it's partly there to confirm the examples in the documentation actually compile

@cary-ilm
Copy link
Member

cary-ilm commented Jul 24, 2020 via email

@lgritz
Copy link
Contributor

lgritz commented Jul 24, 2020

I do really really wish that the library was libOpenEXR.so and not libIlmImf.so. I think that's very counter-intuitive, hard to say out loud, hard to parse in writing because in many fonts you can't tell if it's LLm IIm (eye eye em -- see, you can't tell!) ILm or LIm, and I feel like I've had the awkward conversation 1000 times when I'm talking about libIlmImf (I use its correct name because I'm pedantic) and then have to parenthetically explain, in case someone in my audience doesn't already know what that is, that I'm talking about the library for OpenEXR, but it's not called that.

Strangely, I do not feel strongly about the "Imf" in the names of headers themselves. Maybe because

#include <OpenEXR/IlmImfBlah.h>

is not at all ambiguous, it's quite clear in context that the header is part of OpenEXR. So I feel no urge whatsoever to change the names of the headers.

The namespace issue is trivial to have your cake and eat it, too:

namespace OpenEXR { ... everything ... }

namespace Imf = OpenEXR; // DEPRECATED, keep for back compatibility until the next major release

@oxt3479
Copy link
Contributor Author

oxt3479 commented Jul 24, 2020

Based on the feedback I think I will begin with restructuring using a baseline of the outline posted by Cary. At the same time I'll see what can be done about renaming IlmImf, most likely just changing the namespace and directory names but leaving the same headers.

@cary-ilm
Copy link
Member

cary-ilm commented Jul 24, 2020 via email

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
@kthurston
Copy link

I agree with Peter that the examples shouldn't be in lib, so perhaps

src/examples

instead of src/lib/IlmImfExamples
?

@kthurston
Copy link

If we're going to rename the library, is there a good reason to keep all the sub-folders in separate libraries any more? Meaning maybe we just have libOpenEXR.so as a singular .so instead libIlmImf.so, libIlmThread.so, etc.?

… IlmBase and OpenEXR into one project.

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Signed-off-by: Owen Thompson <oxt3479@rit.edu>
@oxt3479
Copy link
Contributor Author

oxt3479 commented Jul 28, 2020

src

├── bin

│ ├── exr2aces

│ ├── exrbuild

│ ├── exrenvmap

│ ├── exrheader

│ ├── exrmakepreview

│ ├── exrmaketiled

│ ├── exrmultipart

│ ├── exrmultiview

│ └── exrstdattr

├── lib

│ ├── Iex

│ ├── IexMath

│ ├── OpenEXR

│ ├── OpenEXRUtil

│ └── OpenEXRThread

└── test

├── IexTest

├── OpenEXRFuzzTest

├── OpenEXRTest

└── OpenEXRUtilTest

├── OpenEXRExamples
This alternative structure has been applied with the updated PR.

Additionally the IlmBase Library (containing only Iex and IlmThread after the removal of Imath) has been removed and Iex and IlmThread have been moved into the OpenEXR library.

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
…or Imath before including subdirs

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
@meshula
Copy link
Contributor

meshula commented Jul 28, 2020

Looking nicer and nicer. To Cary's point, I also think it's cleaner to move examples out of lib, since examples aren't libraries. It's pretty normal in a repo to see examples as a top level sibling.

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
@cary-ilm
Copy link
Member

Looks good. A few small requests before merging:

  • remove OpenEXR_Viewers entirely.
  • move src/bin/doc to top level "docs"
  • move pdfs from top level "doc" to top level "docs"
  • remove docs/CMakeLists.txt
  • remove src/bin/README.md and src/lib/README.md
  • move src/lib/OpenEXRExamples to src/examples (so that examples is alongside bin, lib, test)

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
… to docs/

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Signed-off-by: Owen Thompson <oxt3479@rit.edu>
…m CMake

Signed-off-by: Owen Thompson <oxt3479@rit.edu>
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good as an initial step, further refinements can come later.

@cary-ilm cary-ilm merged commit 71ed5a3 into AcademySoftwareFoundation:RC-3 Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants