epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

header files in pv/ sub-directories #15

Closed mdavidsaver closed 8 years ago

mdavidsaver commented 8 years ago

At present all header files are installed in a pv/ sub-directory which does not exist in the source tree(s). This is inconvenient as it complicates the configuration of (at least) qtcreator to avoid accidentally editing installed header files which will be overwritten (QtC warns about this, but it's still annoying).

Would there be any objection to moving all header files into pv/ sub-directories in the source tree? That is, creating a pv/ directory in every source directory containing header files.

This would not effect the layout of installed files, and therefore not be user visible.

anjohnson commented 8 years ago

No objections, but I would encourage consistency across all V4 modules. Several modules (e.g. pvaClientCPP) and embedded tops (e.g. pvDatabaseCPP/exampleDatabase) currently implement the subdirectory installation by setting the INSTALL_INCLUDE variable in their configure/CONFIG_SITE file to $(INSTALL_LOCATION)/include/pv, while others (pvCommonCPP/mbSrc, pvDataCPP, pvAccessCPP) set it in their src/Makefile. Of these two I much prefer the latter since the result is limited to the contents of that Makefile.

mdavidsaver commented 8 years ago

Agreed, consistency should be a goal. For what I have in mind there is no need to mess with INSTALL_INCLUDE or similar. Simply setting "INC += pv/xyz.h" is sufficient.

https://github.com/epics-modules/mrfioc2/blob/master/evrApp/src/Makefile#L14

anjohnson commented 8 years ago

Michael's link demonstrates that it will also be necessary to set SRC_DIRS += pv.

My point about INSTALL_INCLUDE was that it has already been messed with in the V4 modules, so that setting will need to be removed at the same time as the files are moved. I was documenting which file some of the modules have used to set it since that may not be obvious.

anjohnson commented 8 years ago

So I just read the meeting minutes; is it really intended that header files will live in a different directory than the .cpp files they describe? There's nothing technically wrong with doing that, but from the perspective of a developer it doesn't seem very nice. My previous comment assumed that you would be moving the implementation files at the same time as the headers...

mdavidsaver commented 8 years ago

@anjohnson wrt SRC_DIRS, yes I had forgotten that. I think this was gives a bit more flexibility. In the case of mrfioc2 headers are installed in three different subdirectories, though not from the same source directory I admit.

mdavidsaver commented 8 years ago

@anjohnson wrt the location of the .cpp files. I wasn't thinking to move them, but could. My purpose was the header files.

For what it's worth, I've found that its common to separate .h and .cpp are in different directories. The cpython interpreter and linux kernel spring immediately to mind. I personally don't find this confusing. It's a bit inconvenient when I'm just using nano, but some might call this self inflicted :)

mdavidsaver commented 8 years ago

@anjohnson wrt SRC_DIRS, turns out this isn't needed.

mdavidsaver commented 8 years ago

For reference, the ugly hack I currently use in "ln -s pv ." in every directory w/ headers except src/pv.

Also, I wasn't completely correct about QtC. For a new "makefile" project it populates .includes with every directory containing headers files, which is now the pv/ sub-dirs. Of course this is easier to fix, just edit .includes and remove /pv from every line.

mdavidsaver commented 8 years ago

forgot the link https://github.com/mdavidsaver/v4workspace/blob/master/qtc-setup.sh#L48

anjohnson commented 8 years ago

@mdavidsaver Yeah, you'd only need to set SRC_DIRS if you also moved the .cpp files into the subdirectory too, or if you kept any private header files there which aren't listed in INC so don't get installed.

mdavidsaver commented 8 years ago

I just realized that in fact this wouldn't be the first mass rename. That honor goes to the pvDataApp/ to src/ change. 19269735aee498b26eead80cdeda09632785784f

dhickin commented 8 years ago

I agree with Andrew that having the headers in a separate subdirectory is not very nice - a little unusual/ugly - but not necessarily bad.

I've been trying both QtCreator and eclipse. I was able to import pvData into both of the IDEs, edit code and get them to build through the IDE.

It wasn't clear to me what the exact problem Michael had was, although I could see the directory structure might present problems.

I can see a little extra configuration is needed to get everything to work optimally. For example if I import pvData into eclipse, add include/pv to the includes files and introduce an error in the header file, when I build, clicking on the error message opens the copy of the header in include/pv in the editor. By adding soft links to src//pv in the src/ directories and adding src/* to the includes you could make it so the original header opens. The new directory structure would avoid the need for the softlinks.

@mdavidsaver, is this the sort of problem you had in mind? I see you had the lines to create the soft links in your Bash script.

If we make the change we should do it across all modules. I'm not completely against this, but it does seem a bit of a big change if we have a workaround. I'd like to to know the cost/benefit of the change, who is going to do the work (Michael?) and are the module owners happy for the changes to be made.

So pro's of the move:

  1. Configuring the IDE is slightly easier.
  2. Although it's a major move in terms of changes, it's probably not that much work,

Cons:

  1. The new structure looks a little unusual/ugly
  2. We can probably achieve the same thing with soft links
  3. It's extra work that doesn't move us forward.
  4. It makes comparison between versions harder. (To be fair git seems to handle this quite well. However it's much harder to compare tar files using a merge/compare tool. Similarly if I import modules into our subversion system or deploy into our production area via a script.)
  5. If people have old checked out modules with local changes merging into the latest also becomes more difficult.
mdavidsaver commented 8 years ago

I give you an Illustrated Guide of the Problems Setting up Qt Creator with pv*

iguide.pdf

mdavidsaver commented 8 years ago

I agree with Andrew that having the headers in a separate subdirectory is not very nice - a little unusual/ugly - but not necessarily bad.

I'm sorry, but this is hardly unusual. In fact every other project I know of which installs headers in a sub-directory also keeps these headers in a sub-directory in the source tree.

mdavidsaver commented 8 years ago

The symlink hack is the only reason the present arrangement is tolerable. However, I don't want to maintain this hack in perpetuity. I don't want to explain it to new developers, and I certainly don't want to have it taken as a template for new code (as it has been thus far).

ralphlange commented 8 years ago

Nice guide!

+1 on moving the headers

mdavidsaver commented 8 years ago

As per todays meeting I think the "question" has been resolved in favor of making this change.

dhickin commented 8 years ago

This has been done for pvDataCPP via push of rebase of #19. Separate pull requests generated for other modules. Closing.

mdavidsaver commented 8 years ago

@dhickin Did you mean to close this ticket?