-
Notifications
You must be signed in to change notification settings - Fork 14
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
Remove the obsolete text from the doc #65
Conversation
doc/install.sgm
Outdated
cloning the repository with the appropriate release tag (each release is | ||
marked with a tag). | ||
The master branch is intended for development use | ||
and can contain an intermediate work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to "and may contain the code in a transitional state."
doc/install.sgm
Outdated
marked with a tag). | ||
The master branch is intended for development use | ||
and can contain an intermediate work. | ||
It is not recommented for use in production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Change "recommented" to "recommended".
While you are at it, could you also remove the Setting |
I agree that setting
|
Sure, but I feel that's part of the |
@esabol I think, in past, there was the scenario when pgsphere was built as a part of postgresql sources tree (contrib/pg_sphere) - lines 88-100 in the top Makefile. Now, pgsphere is not the part of the postgresql tree. I guess, it is why USE_PGXS option was introduced. If we still need to support such scenario we probably should remove USE_PGXS=1 from the makefile and oblige users to set this option in the Makefile as shown in the doc. If we do not need to support such scenario we should cleanup the Makefile, remove obsolete code. |
I think to keep USE_PGXS=1 in the doc because it does nothing. The pg_config is used unconditionally. But, if we decide to support the build in the contribution directory without pg_config, we have to remove the explicit setting of USE_PGXS=1 from the Makefile. It will not affect the developers, who get used to specifying USE_PGXS=1. |
Please don't remove USE_PGXS from the Makefile. People would wish to build pgsphere from within a PG source tree can still use |
@df7cb Yes, sure, i'm not going to remove USE_PGXS=1 from the Makefile without discussions and approvals. I just concerned if to put pgsphere into contrib and to run make world, we will have some problems with USE_PGXS=1 flag in the Makefile. It is why I propose to keep USE_PGXS=1 option in the doc. It may be redundand but it may help us to do the changes in the future. |
I disagree. It is useless and confusing (because it disagrees with the instructions in README). I have no objection to keeping it in the Makefile, however. I just don't think it should be in the documentation. But whatever. It's no big deal. 😄 I compile pgsphere in my |
@esabol pg_config is used to get the path to the postgresql Makefile.global which implements some common rules. When you set USE_PGXS=1, pg_config at your PATH is used to get the path to Makefile.global. It doesn't matter where you put your pgsphere code. If you want to compile pgsphere with using of postgresql Makefile.global that is located in your project tree, then you have to set USE_PGXS=0 to disable pg_config because it can return the path to Makefile.global of another installation. In this case, pgsphere Makefile will try to find the postgresql Makefile at path ../..//src/Makefile.global (line 98 of pgsphere Makefile). pgsphere is not the part of the contrib directory. I'm ok to remove USE_PGXS from the doc. But in this case we have to rewrite the Installation chapter, because there are two build scenarios are described. I propose to keep the only one compilation scenario and to remove the description how to compile in the contrib. What do you think? |
Yeah, I understand how the build system works.
Yes, please. |
@esabol I've updated the Installation section. Could you please verify it? Thank you in advance! |
6727a09
to
d720dc6
Compare
doc/functions.sgm
Outdated
</example> | ||
|
||
<example> | ||
<title>Area of a moc object</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "moc" with "Multi-Order Coverage (MOC)".
doc/install.sgm
Outdated
<programlisting> | ||
<para> | ||
&pgsphere; is not the part of the &postgresql; software. You can download | ||
the latest release sources from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove "sources" from this line. Saying "latest release" is sufficient and more succint.
doc/install.sgm
Outdated
&pgsphere; is not the part of the &postgresql; software. You can download | ||
the latest release sources from the | ||
<ulink url="&pgsphereurl;">&pgsphere; Releases page</ulink>. | ||
The sources can also be downloaded by cloning the repository with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "sources" to "source code". Native English speakers wouldn't use the plural in this context.
doc/install.sgm
Outdated
</programlisting> | ||
|
||
<para> | ||
Compile the code. By default, &pgsphere; is compiled with the HEALPIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "the HEALPIX" to "HEALPix" (no "the" and lowercase "ix").
doc/install.sgm
Outdated
</para> | ||
<programlisting> | ||
</programlisting> | ||
<para>or compile without HEALPIX support:</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "HEALPIX" to "HEALPix" here as well.
doc/install.sgm
Outdated
|
||
<para> | ||
Run regression tests optionally. If the &pgsphere; was compiled without | ||
HEALPIX support, USE_HEALPIX=0 should be specified in make command line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "HEALPIX" to "HEALPix" here as well, but only the first instance. I recommend changing "specified in make command line" to "added after make in the following command".
doc/install.sgm
Outdated
</programlisting> | ||
|
||
<para> | ||
Run regression tests optionally. If the &pgsphere; was compiled without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove "the" before "&pgsphere;".
doc/install.sgm
Outdated
<para> | ||
Install &pgsphere; files to the installation directories. The installation | ||
directories are defined by &pg_config; utility. Superuser (root) access | ||
rights may be required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend adding the setence "If &pgsphere; was compiled without HEALPix support, USE_HEALPIX=0 should be added after make in the following command."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that make install is enough and adding USE_HEALPIX=0 install is not necessary. There are some cases when developer compiles and installs the library using make install command only. In this case USE_HEALPIX should be added to disable the healpix support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is incorrect and you must specify USE_HEALPIX=0
when you make install
, but, if you have tried it and it works, then I'll take your word for it. I'm pretty sure I tried it once and it caused problems, but I might be remembering make test
or another target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esabol You are right, USE_HEALPIX=0 should be added to make install. Otherwise, it will compile moc.c and process_moc.cpp. Anyway, I've modified the doc as you suggested. Thank you!
doc/install.sgm
Outdated
call: | ||
</para> | ||
<programlisting> | ||
<para>To get the version of installed pgSphere software:</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "pgSphere" to "&pgsphere;" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change, please. Thank you!
doc/install.sgm
Outdated
</programlisting> | ||
|
||
<para> | ||
Compile the code. By default, &pgsphere; is compiled with the &healpix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove "the" before "&healpix;" on this line. (Sorry I didn't notice this in my previous review.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esabol It's not a big deal. I've removed the article and rebased the branch. Thank you!
8a03ed4
to
f867318
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you!
f867318
to
7195184
Compare
Rebased |
No description provided.