Skip to content
This repository has been archived by the owner on Sep 5, 2020. It is now read-only.

Source folder layout re-arrangement required #6

Open
leandor opened this issue Oct 16, 2016 · 4 comments
Open

Source folder layout re-arrangement required #6

leandor opened this issue Oct 16, 2016 · 4 comments
Labels
enhancement Request to add or enhance a feature. papi Issue is primarily related to the Python API.
Milestone

Comments

@leandor
Copy link
Contributor

leandor commented Oct 16, 2016

I'd want to make space for introducing separate projects for test cases, and also, break up main CBash sources into static lib packages, with the least possible dependencies with one another.

Also, in the future, I'd want to create a new project for the boost.python generated binding, to replace the current cint API.

My initial idea would be to create a static library for each game which will contain all the record definitions (initially... at least.)

End goal is to speed up a bit the full recompilation of the main CBash project (since the bulk of time recompiling is lost on the individual record files), and also, will help in modularize the source code, as we can use those projects in isolation of one another, and detect unwanted dependencies.

@leandor leandor added the enhancement Request to add or enhance a feature. label Oct 23, 2016
@leandor
Copy link
Contributor Author

leandor commented Oct 23, 2016

Current Source Layout:

  • <root>
  • includes -> public includes (not needed for python)
  • pyd -> new module generates the python bindings
  • src -> original code generates CBash.dll or CBash.lib (static)
    • Oblivion -> TES4File.cpp + all record types
    • Skyrim -> TES5File.cpp + all record types
    • FalloutNewVegas -> FNVFile.cpp + all record types
  • tests -> tests cases for produced output

Goal is to split 'core' from games, and generate static libs from each subgroup.

So, future layout I'm looking forward (conceptually, at least...) to have:

  • root
    • includes -> public includes (not needed for python)
    • src
      • common -> (aka library, or shared) holds utils, base classes, common declarations, etc. (game/<game> below can depend on these. Probably 7z wrapping will become clases inside this... can have subdirs as needed.)
      • game/<game> -> holds specific game's declarations (can depend on common but not on core)
      • core -> holds rest of the existing API, mostly classes that handle loading and traversing, must depend on specific game (since requires parsing using specific game's File.cpp
      • <new api> -> new module (above called 'pyd', better name needed) which links with all above and wraps/adapts API to Python. Depends on core, and by transitivity on all of the above.

Conceptually, each folder under src is a component or separated module.

  • Each folder inside src will generate a project target and will compile to a static lib, producing an intermediate output.
  • Each successive link afterwards will link to the required dependency above, and in time produce a new static lib.
  • PRO: Compilation isolation for separated code concerns, each module/layer can be compiled on its own and only recompiled when a change is done on one of its files.
  • PRO: Clear boundaries to limit cross-references between source files where they wouldn't be noticed otherwise.
  • CON: Link errors won't show up until later in the compilation, when everything needs to be resolved to produce the actual binary output. (I guess? Not sure on this)

Points of debate, contention, and doubts:

  • I'm not totally clear that I'd want all the source files dumped into each main component folder, since I'd like to have some C++ tests too.
    • Should I put all the C++ tests on a separated folder inside root and divide them between the test concerning the public facing API, and the internal C++ tests, or,
    • Should I add an internal folder subdivision to each component, like core/src and core/tests.
      • -> (I don't like the repetition of 'src' in src\core\src if that's the case.)
    • Or, perhaps I could have ALL files dumped on each component main folder and have the tests be called _tests.cpp.
    • Still undecided 😬
    • Reason for wanting c++ level tests?
      • Public facing API tests need full mod files to work, specifically need a game master, or simile. We can probably provide fake files to test specific stuff, but they would have to be created one by one extracting data from a valid source.
      • OTOH, I can probably mock up all the non-essential dependencies when a test for a class that is small enough not to require an enormous job at mocking EVERYTHING in CBash to be able to work.
      • I.e. c++ tests can be more focused on assurance that tool classes are working correctly, than on the data structure verification.
  • Should games be a category outside core? Maybe they belong to that layer.
    • I'd like to keep the games separated, to avoid inter-dependency pollution, since I like my layer subdivision to be perfectly clear.
    • Whatever I do it should be perfectly clear that declarations in a game folder can't depend on declarations on core classes/headers.
    • That means that any dependency like that needs to be moved to common and abstracted away.
    • Maybe this indicates that I need to do this layout change in two steps,
      1. extract common layer and leave core and games as they are
      2. later extract games one by one checking for cross-dependency and fix on a case by case basis.
    • Question that arises: Is common big enough to justify its presence?
      • Superficially, for what I saw of the code, it seems that it indeed is. But, not sure yet.
  • Should a single CBash binary contain ALL supported games or should I link to different games depending on conditional declarations, and provide different CBash binaries for each supported game?
    • That's easily doable. btw. I can generate as output $Config\$Game\CBash.pyd where $Config is Relase or Debug and $Game is one of the supported games.

@leandor leandor added review Review from another developer is requested. papi Issue is primarily related to the Python API. labels Oct 23, 2016
@Utumno
Copy link
Member

Utumno commented Oct 24, 2016

Can't readily review that - even more so as I have similar questions on py tests layout - I keep asking:

http://stackoverflow.com/questions/34694437/python-test-package-layout

Will have a closer look - but I guess that's a trial and error approach problem - so see what's in the code, how it splits better/compiles faster etc

I have extensive refactoring experience though, so I could have a look soonish - just this week will be pressed

Nice work on labels btw ! This repo is becoming my little garden of order and beauty in a chaotic wrye Bash world :)
You may want to look here https://github.com/wrye-bash/wrye-bash/wiki/[github]-Creating-and-Managing-Issues

@leandor
Copy link
Contributor Author

leandor commented Oct 24, 2016

Good point, yes. And no worries, take your time, no hurry. I'm not going to do anything big before analyzing/thinking on the consequences.

http://stackoverflow.com/questions/34694437/python-test-package-layout

((BTW, I really liked the idea I saw on one of the answers there of putting tests inside the same code folder, but inside a tests/ directory. So, maybe I'll do that for the C++ tests. It simplifies the folder structure quite a bit, by reducing it by one level, basically.))

I think I'll take the plunge and do the renaming locally, I'll try the different things and see which one produces the most cleaner layout.

I'll do that after getting at least one released binary in the old format (CBash.dll) so we can at least test it and see if everything is working as it should, before starting doing too much changes at once.


Nice work on labels btw ! This repo is becoming my little garden of order and beauty in a chaotic wrye Bash world :)

Thanks! I shamelessly copied them from your main repo :)

You may want to look here https://github.com/wrye-bash/wrye-bash/wiki/[github]-Creating-and-Managing-Issues

Thanks! Haven't seen that one... need to dive more into the wikis, it seems. And I totally misunderstood the use of the TODO. I tagged all issues that have check-lists with TODO, but the actual use-case is very specific.

And going a bit off topic here, so pardon me:

I thought about having three-layered tags at most, like one label tags the "module" (for now: Bash API & docs), the next is the type (bug, enh, etc.) and the last is a flag/modifier, like review/goal/etc.

No more than three static label components per issue, there can be more when they tag a transitory state (like question, and also, I thought about adding a WIP tag to mark what I'm doing ATM, for example) but I think that's better handled with milestones and/or projects, right?

@Utumno
Copy link
Member

Utumno commented Oct 25, 2016

but I think that's better handled with milestones and/or projects, right?

Yep !

Re: three levels - good idea letr me ponder - the labels is still wip on main repo but mostly settled - I have thought long and hard, trial and error etc

leandor added a commit that referenced this issue Nov 3, 2016
Replace old deprecated C headers with the C++11 compliant one.
leandor added a commit that referenced this issue Nov 3, 2016
(This will make a big mess, that's why I'm doing it before anything
else.)

All tis commit does is to move game specific sources to a sub-folder
`src/game` instead of it being all dumped on the `src` folder.
leandor added a commit that referenced this issue Nov 3, 2016
Moved source files that aren't dependend on game specifics to
folder `src/common` and made a static library out of them.
leandor added a commit that referenced this issue Nov 3, 2016
Moved sources that are specific to the C api defined for CBash.dll to a
sub-folder called `src/capi`.
leandor added a commit that referenced this issue Nov 3, 2016
Moved source files that depend on `game` but aren't part of the C API to
folder `src/main` and made a static library out of them.
leandor added a commit that referenced this issue Nov 3, 2016
Added CMakeLists.txt build rules to `src/common`, `src/capi` and
`src/main`.

Added build rules for creating static library targets for FalloutNV,
Oblivion and Skyrim to folder `src/game/<game>` .

Added a fake intermediate library `src/game` that serves as a quick
referral for adding dependences to the three implemented games
indirectly. For this I required to add a dummy empty `game.cpp`
file as CMake doesn't accept libraries with no source code.

Also adjusted pre-existing CMakeLists.txt for the new folder
structure and static libraries usage.
leandor added a commit that referenced this issue Nov 3, 2016
Adjusted `#include`s not to depend on relative folder structure and
instead be based on module of origin, as this:

`#include "<module>/<header>.h"`

This is allowed by the fact that the `src` directory is added to the
INCLUDES search path.
leandor added a commit that referenced this issue Nov 3, 2016
Split the content of the file `src/GRUPRecord.h` into three pieces and added
each corresponding part to the existing `src/game/<game>/GRUPRecord.h` that
was already in existance.

After that the original file was empty, so it was deleted.

Also the `src/game/<game>/GRUPRecord.h` files now required explicit inclusion
of `Allocator.h` since the content that was moved to them depended on the
class template declared there.
leandor added a commit that referenced this issue Nov 3, 2016
Moved the game-specific game-specific Record out of the file
`src/common/GenericRecord.h` and into new files named after each
class' name, on the `src/game/<game>` folder.
leandor added a commit that referenced this issue Nov 3, 2016
Skyrim and FalloutNV records now depend on the new specific game record
header that was accesible before by just including `GenericRecord.h`.

Those are TES5Record.h for Skyrim's records and FNVRecord.h for
FalloutNV's ones.
leandor added a commit that referenced this issue Nov 3, 2016
Moved Skyrim's specific String functionality that was located in the
generic master record (TES4Record) to the Skyrim ModFile implementation,
as it's only used from records belonging to Skyrim's implementation.

For that I had to break a cyclic dependence on TES5Record <-> TES5File
by introducing a new zero sized abstract class TES5ModFile that only
publishes the method required to access the StringsLookup
implementation.

As a result of this I can remove the inclusion of SkyrimCommon.h
from `TES4Record.cpp`
leandor added a commit that referenced this issue Nov 3, 2016
Modified implementation of code that required access to the
LookupStrings to use the new location, by accessing it from the parent
ModFile.

NOTE: I left some code commented for now since I don't want to lose it,
but I didn't see it used from anywhere, so I don't know yet the proper place
where to move them. I supposed the logical place should also be the game's
ModFile class.
leandor added a commit that referenced this issue Nov 3, 2016
Added CMakeLists.txt build rules to `src/common`, `src/capi` and
`src/main`.

Added build rules for creating static library targets for FalloutNV,
Oblivion and Skyrim to folder `src/game/<game>` .

Added a fake intermediate library `src/game` that serves as a quick
referral for adding dependences to the three implemented games
indirectly. For this I required to add a dummy empty `game.cpp`
file as CMake doesn't accept libraries with no source code.

Also adjusted pre-existing CMakeLists.txt for the new folder
structure and static libraries usage.
leandor added a commit that referenced this issue Nov 3, 2016
Adjusted `#include`s not to depend on relative folder structure and
instead be based on module of origin, as this:

`#include "<module>/<header>.h"`

This is allowed by the fact that the `src` directory is added to the
INCLUDES search path.
leandor added a commit that referenced this issue Nov 3, 2016
Split the content of the file `src/GRUPRecord.h` into three pieces and added
each corresponding part to the existing `src/game/<game>/GRUPRecord.h` that
was already in existance.

After that the original file was empty, so it was deleted.

Also the `src/game/<game>/GRUPRecord.h` files now required explicit inclusion
of `Allocator.h` since the content that was moved to them depended on the
class template declared there.
leandor added a commit that referenced this issue Nov 3, 2016
Moved the game-specific game-specific Record out of the file
`src/common/GenericRecord.h` and into new files named after each
class' name, on the `src/game/<game>` folder.
leandor added a commit that referenced this issue Nov 3, 2016
Skyrim and FalloutNV records now depend on the new specific game record
header that was accesible before by just including `GenericRecord.h`.

Those are TES5Record.h for Skyrim's records and FNVRecord.h for
FalloutNV's ones.
leandor added a commit that referenced this issue Nov 3, 2016
Moved Skyrim's specific String functionality that was located in the
generic master record (TES4Record) to the Skyrim ModFile implementation,
as it's only used from records belonging to Skyrim's implementation.

For that I had to break a cyclic dependence on TES5Record <-> TES5File
by introducing a new zero sized abstract class TES5ModFile that only
publishes the method required to access the StringsLookup
implementation.

As a result of this I can remove the inclusion of SkyrimCommon.h
from `TES4Record.cpp`
leandor added a commit that referenced this issue Nov 3, 2016
Modified implementation of code that required access to the
LookupStrings to use the new location, by accessing it from the parent
ModFile.

NOTE: I left some code commented for now since I don't want to lose it,
but I didn't see it used from anywhere, so I don't know yet the proper place
where to move them. I supposed the logical place should also be the game's
ModFile class.
leandor added a commit that referenced this issue Nov 3, 2016
Moved source files that generate the python binding to folder
`src/papi` (i.e. "Python API").
leandor added a commit that referenced this issue Jul 1, 2017
Replace old deprecated C headers with the C++11 compliant one.
leandor added a commit that referenced this issue Jul 1, 2017
(This will make a big mess, that's why I'm doing it before anything
else.)

All tis commit does is to move game specific sources to a sub-folder
`src/game` instead of it being all dumped on the `src` folder.
leandor added a commit that referenced this issue Jul 1, 2017
Moved source files that aren't dependend on game specifics to
folder `src/common` and made a static library out of them.
leandor added a commit that referenced this issue Jul 1, 2017
Moved sources that are specific to the C api defined for CBash.dll to a
sub-folder called `src/capi`.
leandor added a commit that referenced this issue Jul 1, 2017
Moved source files that depend on `game` but aren't part of the C API to
folder `src/main` and made a static library out of them.
leandor added a commit that referenced this issue Jul 1, 2017
Added CMakeLists.txt build rules to `src/common`, `src/capi` and
`src/main`.

Added build rules for creating static library targets for FalloutNV,
Oblivion and Skyrim to folder `src/game/<game>` .

Added a fake intermediate library `src/game` that serves as a quick
referral for adding dependences to the three implemented games
indirectly. For this I required to add a dummy empty `game.cpp`
file as CMake doesn't accept libraries with no source code.

Also adjusted pre-existing CMakeLists.txt for the new folder
structure and static libraries usage.
leandor added a commit that referenced this issue Jul 1, 2017
Adjusted `#include`s not to depend on relative folder structure and
instead be based on module of origin, as this:

`#include "<module>/<header>.h"`

This is allowed by the fact that the `src` directory is added to the
INCLUDES search path.
leandor added a commit that referenced this issue Jul 1, 2017
Split the content of the file `src/GRUPRecord.h` into three pieces and added
each corresponding part to the existing `src/game/<game>/GRUPRecord.h` that
was already in existance.

After that the original file was empty, so it was deleted.

Also the `src/game/<game>/GRUPRecord.h` files now required explicit inclusion
of `Allocator.h` since the content that was moved to them depended on the
class template declared there.
leandor added a commit that referenced this issue Jul 1, 2017
Moved the game-specific game-specific Record out of the file
`src/common/GenericRecord.h` and into new files named after each
class' name, on the `src/game/<game>` folder.
leandor added a commit that referenced this issue Jul 1, 2017
Skyrim and FalloutNV records now depend on the new specific game record
header that was accesible before by just including `GenericRecord.h`.

Those are TES5Record.h for Skyrim's records and FNVRecord.h for
FalloutNV's ones.
leandor added a commit that referenced this issue Jul 1, 2017
Moved Skyrim's specific String functionality that was located in the
generic master record (TES4Record) to the Skyrim ModFile implementation,
as it's only used from records belonging to Skyrim's implementation.

For that I had to break a cyclic dependence on TES5Record <-> TES5File
by introducing a new zero sized abstract class TES5ModFile that only
publishes the method required to access the StringsLookup
implementation.

As a result of this I can remove the inclusion of SkyrimCommon.h
from `TES4Record.cpp`
leandor added a commit that referenced this issue Jul 1, 2017
Modified implementation of code that required access to the
LookupStrings to use the new location, by accessing it from the parent
ModFile.

NOTE: I left some code commented for now since I don't want to lose it,
but I didn't see it used from anywhere, so I don't know yet the proper place
where to move them. I supposed the logical place should also be the game's
ModFile class.
leandor added a commit that referenced this issue Jul 1, 2017
Moved source files that generate the python binding to folder
`src/papi` (i.e. "Python API").
@Infernio Infernio added this to the 0.8.0 milestone Sep 20, 2019
@Infernio Infernio removed the review Review from another developer is requested. label Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Request to add or enhance a feature. papi Issue is primarily related to the Python API.
Projects
None yet
Development

No branches or pull requests

3 participants