Page MenuHomePhabricator

build: move config/ to data/elementary/config
ClosedPublic

Authored by zmike on Apr 27 2018, 12:30 PM.

Details

Summary

this is part of the datadir distribution, it should not be in a different
directory than the rest of the datadir distribution

the gnu coding standards (https://www.gnu.org/prep/standards/html_node/Directory-Variables.html)
define 'datadir' as:

The directory for installing idiosyncratic read-only architecture-independent
data files for this program. This is usually the same place as ‘datarootdir’,
but we use the two separate variables so that you can move these program-specific
files without altering the location for Info files, man pages, etc.

This should normally be /usr/local/share, but write it as $(datarootdir).
(If you are using Autoconf, write it as ‘@datadir@’.)

The definition of ‘datadir’ is the same for all packages, so you should install your
data in a subdirectory thereof. Most packages install their data under $(datadir)/package-name/.

while this text has no clear requirement or suggestion for a corresponding
repository layout, projects typically employ a certain consistency in their
repository layout both for ease of maintenance and ease of learning for new
contributors.

this project has both a data/ directory, which contains the datadir distribution,
as well as the config/ directory, which also contains the datadir distribution.
this complicates matters both for active maintainers/developers who must
remember that the repository and build tree layouts have this exception,
and for new contributors who will initially be confused by this exception

other well-organized open source projects, such as wayland, have chosen to not
use a data/ directory. these projects have the datadir distribution in the base
directory of the repositor, which is a fine practice as it maintains consistency
for the project since all the files for the datadir distribution are in the same
directory.

by applying this patch, the project will move towards a more easily readable and
learnable layout. current and future developers will no longer need to wonder why
this directory is outside of the data/ directory, and anyone attempting to reference
these files from the source/build trees will be able to do so more easily

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zmike requested review of this revision.Apr 27 2018, 12:30 PM
zmike created this revision.
zmike added a project: efl.May 2 2018, 4:47 PM
stefan_schmidt accepted this revision.May 3 2018, 5:25 AM

I am ok with this change (having the config dir always looked oddly split out), but I would like to hear if Cedric or Raster see any issue with it I missed.

This revision is now accepted and ready to land.May 3 2018, 5:25 AM
raster requested changes to this revision.May 4 2018, 7:59 PM

enlightenment still has its config in config... so why be different? can you explain bigger picture than just a one liner and explain why you want e and efl to be inconsistent?

This revision now requires changes to proceed.May 4 2018, 7:59 PM
zmike added a comment.May 8 2018, 8:00 AM

enlightenment still has its config in config... so why be different? can you explain bigger picture than just a one liner and explain why you want e and efl to be inconsistent?

This question seems misleading. It is, in fact, all efl-related projects which are inconsistent. The standard source tree layout puts 'data' files into the data/ directory; this is why autotools has named the option related to distribution of files in it --datadir.

Having files from the datadir distribution outside of data/, where the rest of the datadir distribution resides, leads to confusion when new developers try to find the files in the tree based on expectations from previous projects. It also presents inconsistencies when reading code related to use of these files: does something which references a file in config/ use the datadir-related functions or is there a separate function for finding this file? The only way to know is to then have to read the makefile, which should not be necessary.

In summary, there is no 'bigger picture' and the commit log is exactly as descriptive as it needs to be for this case since datadir is a defined, searchable term which encapsulates all of the above.

raster added a comment.May 9 2018, 7:57 AM

well that data dir inside the share dir is actually kind of a mistake. it rally was meant to just go into the PREFIX/share/projname dir. it was a lazy mistake that was never fixed/changed in e.

but the src trees of efl and e don't match the installed trees. there are semantics on how it's laid out because that's just how it was built. it does make it easy enough to find what you want (especially once you get used to the layout). eina prefix can't deal as-is with the src tree because it's not an exact mirror of the install tree. it has always bee special-cased as a result. modules, libs and bins.

so if your goal is to use eina prefix without special cases then a lot more work needs to be done in the tree. it has to be totally redone and we cant have subdirs for libs and binaries. if your goal is to have less specialization then i guess it does reduce it, but that brings up my other issue. inconsistency. e and efl will not be the same layout. inconsistency is bad. it's a learning curve and a magnet for mistakes when you forget "which style was that again?" :)

so if there is no bigger picture like changing everything to be the same, then i say no. better consistent what we have than inconsistent and a little less special casing (as it doesn't remove special casing entirely).

zmike added a comment.May 9 2018, 8:13 AM

Citing another (mostly unrelated unrelated) repository layout as a valid reason to reject changes to this one seems short-sighted at best. Since you bring it up, however, my points from the above post apply to any other repo on our infrastructure which you might mention; being consistent across our own projects is fine, but having a lower barrier to entry through standardization is better. If you require consistency across projects, it's trivial enough to post patches for those moves at this time. Rejecting this patch because another repository has yet to be changed in a similar way is nothing more than a catch-22.

I've made no mention of eina prefix usage here, so I don't know why that was brought up. I also made no mention of src/ directory layout not matching installed layout, so this is another question mark for me.

To speed things up, please stop trying to read "between the lines" in my posts. If I had something additional to say, I would be explicit.

raster added a comment.May 9 2018, 9:04 AM

the day enlightenment and efl are mostly unrelated is the day pigs fly. you implied prefix with "use the datadir-related functions" (i quote) because it's the eina prefix subsystem that figures out where to find these both at compile and runtime.

as for different projects and standardization, then look around. most projects don't use a data dir at all. they also don't use src/bin, src/lib, src/modules etc. ... and if they use src it will maybe only have the core code there with tools and clients in other toplevel dirs. so if your argument is we should just do what everyone else does then it's major surgery to the src tree and my take is the tree is worse off as a result, not just moving the config dir into data.

as for matching install - that was your point - when code looks for the files, it looks in the install destination based on what eina_prefix would say (or has a fake was of pretending its installed when in the src tree to go find it), so your argument is that since it looks for it there in the src code then it should be there in the src tree. i'm not reading in between the lines. i'm working with the direct conclusions of your logic.

you want to change the location of these files in one of our primary core projects and not in the other. you could have said "well i'll change both to match" and there would be no argument. yes. consistency is important. until my last reply you didn't say anything about making like changes elsewhere. i'm rejecting this until there are positive signs in this direction. welcome to code review. you are the one who says that it's great. things would have been fast if your first response was "ok - to fix consistency, here is an equivalent patch to e. so the ball has been entirely in your court all along to speed things up.

also explain more in your commit logs. yours are extremely very often and do not provide much information when going through them. this absolutely did not provide enough information or explanation.

zmike added a comment.EditedMay 10 2018, 8:45 AM

I'm going to be brief since I've already gone over my allotted time budget for keyboard use today:

it's the eina prefix subsystem that figures out where to find these both at compile and runtime.

This must not be the case when running from the build directory.

as for different projects and standardization, then look around. most projects don't use a data dir at all. they also don't use src/bin, src/lib, src/modules etc. ... and if they use src it will maybe only have the core code there with tools and clients in other toplevel dirs. so if your argument is we should just do what everyone else does then it's major surgery to the src tree and my take is the tree is worse off as a result, not just moving the config dir into data.

My argument is that other projects which use data/ for datadir distribution are consistent with themselves. If a project doesn't use data/ then the datadir distribution is generally in the root source directory: this is as consistent as having it in data/.

as for matching install - that was your point - when code looks for the files, it looks in the install destination based on what eina_prefix would say (or has a fake was of pretending its installed when in the src tree to go find it), so your argument is that since it looks for it there in the src code then it should be there in the src tree. i'm not reading in between the lines. i'm working with the direct conclusions of your logic.

No, this is just not the case. I've never mentioned install locations except to refute your claim that I had mentioned install locations. This patch is about source/repository tree layout.

you want to change the location of these files in one of our primary core projects and not in the other. you could have said "well i'll change both to match" and there would be no argument. yes. consistency is important. until my last reply you didn't say anything about making like changes elsewhere. i'm rejecting this until there are positive signs in this direction.

To me, your review comments read as "No." and not "Conditionally yes if you also do X." Why would I propose a second patch if the first has already been unconditionally rejected? I've created D6154 for this.

welcome to code review. you are the one who says that it's great.

I'm not sure if this was as intentionally hostile as it comes across, but in either case I don't appreciate this type of response and I would prefer if you could avoid it in the future.

things would have been fast if your first response was "ok - to fix consistency, here is an equivalent patch to e. so the ball has been entirely in your court all along to speed things up.

It seems that you misinterpreted my above reference to 'speed'. This is a trivial patch and it doesn't matter to me if it lands today or in 10 years. Speed in this case was related to the time it would take me at a keyboard to compose replies, since your comments tend to double in size with each new post.

also explain more in your commit logs. yours are extremely very often and do not provide much information when going through them. this absolutely did not provide enough information or explanation.

Thanks for this feedback, I'm always looking for ways to improve myself as an engineer.

zmike updated this revision to Diff 14597.May 10 2018, 8:46 AM
zmike edited the summary of this revision. (Show Details)

This must not be the case when running from the build directory.

well to be exact - eina prefix itself doesn't do it but the code directly using eina prefix does. grep for EFL_RUN_IN_TREE. it special cases when running in tree vs funding otherwise. elm actually doesn't even do this for config files so it's going to fail to load them if run in tree and thus just rely on compiled in defaults. but my point is that there is special casing for running n the tree. it's littered through efl so things can be used without being installed and run from the tree. so whatever code per lib that uses eina prefix to find stuff generally has a special run in tree exception and it's horrible to maintain because it is a special case, but it works.

My argument is that other projects which use data/ for datadir distribution are consistent with themselves. If a project doesn't use data/ then the datadir distribution is generally in the root source directory: this is as consistent as having it in data/.

i don't think this is as widely spread as you. "data" dirs are unusual in my experience with src trees. i'm not going to waste my time finding src trees of lots of projects to do some statistics one way or another, but my general experience is that data can be spread around lots of dirs along with libs, tools and other src in various projects. there really isn't a convention or standard. it's project by project.

No, this is just not the case. I've never mentioned install locations except to refute your claim that I had mentioned install locations. This patch is about source/repository tree layout.

that is false. your REASONING is that this has to move into the data dir BEFORE of its install location. the data dir gets installed to prefix/share/xxx and config too and thus it must be there because of the INSTALL LOCATION. you said it yourself. your first answer as to why i quote:

Having files from the datadir distribution outside of data/, where the rest of the datadir distribution resides

So what do you expect when you yourself say that, and then you later say you didn't say it? It is the first thing you said with an explanation as to why... and I begin to get annoyed when this happens. Or do you think that your answer to me was a refute? Do you think that if i pose a question to get more information your job is to instantly try and refute it? That is precisely the kind of message i'm getting within this conversation here. combined with "my commit log is just fine and dandy so deal with it" attitude.

To me, your review comments read as "No." and not "Conditionally yes if you also do X." Why would I propose a second patch if the first has already been unconditionally rejected? I've created D6154 for this.

first. your commit logs here are poor. they do not explain much at all. i ask and put a reject in because your are making a change that is both inconsistent with other trees and has no reason why that is worth it's salt in the logs. you seems to take umbrage then when asked denying no bigger picture when there is as you go into explaining it then, then you deny you explained it as above (consistency with install dir), which is what you said. when someone rejects a patch with "reason x" then a solution is to FIX reason x. as an example: "i reject this patch because this function is missing documentation" ... well what to you think the solution is? do you need to be told "well go add documentation"? if you read what i have been saying - i see value in possibly making other special cased code simpler with the general idea but that it can't fully solve that problem, so i'm seeing some value in it and perhaps leaving it to you to propose a better way that maximizes or fixes that problem too (getting rid of run in tree stuff). why do you have to be so difficult and decide to not use your intelligence and do the blatantly obvious thing?

I'm not sure if this was as intentionally hostile as it comes across, but in either case I don't appreciate this type of response and I would prefer if you could avoid it in the future.

i believe your snideness does this. like with "To speed things up, please stop trying to read "between the lines" in my posts. If I had something additional to say, I would be explicit.". i'm not reading between lines. i'm following through with the natural code logic and implications. your originally rather hostile "In summary, there is no 'bigger picture' and the commit log is exactly as descriptive as it needs to be". this along with some other things sets the tone for the conversation. once you start with that then you can expect to be bitten back when you start disliking the speed with push-back.

i've told you multiple times that your attitude and way of communicating has become incredibly abrasive and snide several times before and the above is a perfect example. you are absolutely false in saying there is no bigger picture. the fact that there has to be a long explanation and conversation about this to get details that are not in the commit log alone says that. the fact i asked about that says that the log does not have enough information. your instant response of "well that's enough. deal with it", and that's the tone you emit again and again, so don't be surprised if people alter their tone to match yours as it seems to be the way you like to communicate, our attitude just drags it out.

your old "deal with it" sunglass gifs etc. were amusing for a while because they came with a whole playful attitude and could be taken in that spirit. now its the "deal with it" attitude all wrapped in "super professional seriousness" and it is absolutely no longer funny at all.

now... there is a longer log and another patch so... back to sometnnig positive.

raster accepted this revision.May 11 2018, 2:26 AM

ok. good log. probably more log than needed, but better indeed. and a lick patch for e. feel free to push when ready

This revision is now accepted and ready to land.May 11 2018, 2:26 AM

I will get this and D6014 merged now

This revision was automatically updated to reflect the committed changes.