- Notifications
You must be signed in to change notification settings - Fork15
Remove the obsolete text from the doc#65
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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. |
esabol commentedSep 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
tod720dc6
Comparedoc/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 specifyUSE_HEALPIX=0
when youmake 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 rememberingmake 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!
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!
Rebased |
No description provided.