search

citusdata / pg_shard

ATTENTION: pg_shard is superseded by Citus, its more powerful replacement
https://github.com/citusdata/citus
GNU Lesser General Public License v3.0
1.06k stars 63 forks source link

Reorganize project files #116

Closed jasonmp85 closed 9 years ago

jasonmp85 commented 9 years ago

In the course of working on metadata sync, I realized that we the top-level directory of this project is essentially a huge grab bag of files. Based on guidance from PGXN and inspiration from kv-pair, pg-semver, and pg_partman, I've reworked our project's structure to be a bit… saner.

In particular, source files are in src, headers in include, SQL files in sql, and updates in updates. test contains test-related files, including the test launcher, SQL test files (now numbered), expected results, and supporting C files. Docs are in doc, executables in bin, and subdirectory Makefiles have been introduced where appropriate.

The overarching Makefile is much simpler and I foresee far fewer edits to it from now on.

jasonmp85 commented 9 years ago

@onderkalaci putting you on this for now. I tried to keep semantic changes to a minimum and just move around files where possible (though such moves did require Makefile changes, etc.)

@ozgune had talked about doing something like this before the release to make our project look nicer on GitHub (not so many top-level files before you get to the README.md, etc.)

I think I might create a config folder for tool config (.iwyu, .uncrustify, etc.) and that should also help a bit going forward.

jasonmp85 commented 9 years ago

I've rebased this for a cleaner diff, but for obvious (?) reasons, don't merge until we get the pushdown, enumeration, and metadata branches merged (I'll finish looking at those changes tonight).

onderkalaci commented 9 years ago

Hey @jasonmp85 ,

I think this change is great and makes the file layout much readable. I have couple of comments/questions:

I mostly concentrated on the changes in Makefile. Well, this Makefile seems high-tech compared to the old one:)) As you stated, others are mostly file movements. Below are my comments on Makefile changes:

jasonmp85 commented 9 years ago
  • I guess creating a config folder would make the change more complete.

I agree. I'll do it.

  • I don't think it is common, but, what about putting LICENSE, CONTRIBUTE.md and CHANGELOG.md into a separate folder? This is just an open question.

CONTRIBUTING is required at the root level to have GitHub link users to contribution guidelines, and LICENSE, while not as cut-and-dry, is added at that location for new projects. In case any tools expect it there, I think we should leave it.

In fact, most of these files are pretty common at the root level: what's the last open-source project you downloaded that didn't have a INSTALL, README, LICENSE, HISTORY, and maybe COPYRIGHT file at the root level? Leaving these as-is.

Well, this Makefile seems high-tech compared to the old one

Most of the changes I made get rid of extra steps and rely on more wildcards or implicit rules already present in the PGXS system. Anything extra probably was inspired by PGXN's documentation as commented in the pull request description.

  • Previously pg_shard were compiling only one of the ruleutiles file. But, with this change, we compile and link both ruleutils files. Why is that? Wouldn't it cause a problem?

Nope! I added an #if macro guard to entirely remove one file or the other based on PG_VERSION_NUM. In my opinion, this is way simpler than detecting the version number in the Makefile and building one or the other into ruleutils.o. Now both are built, though one is always empty depending upon the version of PostgreSQL in use.

  • REGRESS_OPTS seems to have changed. Is that really required? (ie: --inputdir=test --load-language=plpgsql)

The load-language thing is something I've seen in some other projects. Probably not necessary, but it will eagerly load the PL/pgSQL language, which we need during testing. Might as well have it: many other projects do.

inputdir is _definitelyneeded, as it says where the test folders (sqlandexpected) live. Since I've moved those into atest` directory, we need it.

Running regex for finding the extension name makes me feel uncomfortable a bit . Couldn't we just write pg_shard there? It makes more sense to me since we are not going to change it potentially.

Yeah, it does seem silly, but I believe PGXN tools generate this line now so it's pretty standard. I see it as boilerplate; it's not something we wrote, it was written by tools (pgxn_utils skeleton, I believe).

  • Previously, we didn't have DOC in the Makefile. Do we need it?

It's a supported variable for PGXS, so we might as well point it at our docs. Less important at the moment; might be more meaningful once we have more impressive docs. Also generated by PGXN tooling.

  • I am not good at regex. So, I don't know how can assure this. But, if it works, it seems it will always work, right?

Again, this comes from pgxn_utils skeleton's generated Makefile and is used in many other extensions, some of which I linked in the description for this pull request. And from what I can tell, it's a good regex for what's needed.

To play around with regular expression matching, this or this can be useful.

jasonmp85 commented 9 years ago

Actually, I only see two tool files in the root, so pushing them into their own directory doesn't seem worth it at this time. I'll reassess later. This is a good first step.

In case you're wondering why the only upgrade SQL files are checked in, it's because my grand vision has the most recent install file generated from a bunch of smaller SQL files (to permit us keeping our PL/pgSQL in clearly-named-and-documented files), so the pattern from now on will be "generate the most recent install file but freeze upgrades in time and check them in".

jasonmp85 commented 9 years ago

Merging this. I'll do the (annoying) work of rebasing the outstanding branches.