Remove the obsolete text from the doc #65

vitcpp · 2023-09-19T10:05:02Z

No description provided.

esabol · 2023-09-19T14:11:47Z

doc/install.sgm

+                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.


Change to "and may contain the code in a transitional state."

esabol · 2023-09-19T14:13:15Z

doc/install.sgm

+                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.


Typo. Change "recommented" to "recommended".

esabol · 2023-09-20T15:04:22Z

While you are at it, could you also remove the USE_PGXS=1 from the make commands on lines 75 and 81 of doc/install.sgm? That's completely outdated and unnecessary.

Setting PG_CONFIG still works, but that's mostly unnecessary as well.

df7cb · 2023-09-20T15:47:37Z

I agree that setting PGXS on the command line is obsolete since the Makefile already contains it, but setting PG_CONFIG is still the primary interface to selecting the target PG version.

make PG_CONFIG=/usr/lib/postgresql/15/bin/pg_config

esabol · 2023-09-20T16:07:50Z

Sure, but I feel that's part of the postgres user's environment in most cases. At least I always have it set, and I always update that setting whenever I install a new major version release. So I stand by my statement that setting it in the make command is "mostly unnecessary", but I have no objection to keeping it in the documentation.

vitcpp · 2023-09-21T05:21:58Z

@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.

vitcpp · 2023-09-21T06:43:21Z

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.

df7cb · 2023-09-21T07:09:28Z

Please don't remove USE_PGXS from the Makefile. People would wish to build pgsphere from within a PG source tree can still use make USE_PGXS=. But I've never seen anyone do that, so let's not bother the other 99.9% of the users with that flag.

vitcpp · 2023-09-21T07:53:48Z

@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 · 2023-09-21T18:24:47Z

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 contrib directory all the time, and it compiles just fine with USE_PGXS=1 set in the Makefile.

vitcpp · 2023-09-21T19:40:10Z

@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?

esabol · 2023-09-22T02:44:10Z

Yeah, I understand how the build system works.

I propose to keep the only one compilation scenario and to remove the description how to compile in the contrib.

What do you think?

Yes, please.

vitcpp · 2023-09-22T09:18:47Z

@esabol I've updated the Installation section. Could you please verify it? Thank you in advance!

esabol · 2023-09-24T20:24:01Z

doc/functions.sgm

+            </example>
+
+            <example>
+              <title>Area of a moc object</title>


Replace "moc" with "Multi-Order Coverage (MOC)".

esabol · 2023-09-24T20:25:55Z

doc/install.sgm

-            <programlisting>
+  <para>
+   &pgsphere; is not the part of the &postgresql; software. You can download
+   the latest release sources from the


Remove "sources" from this line. Saying "latest release" is sufficient and more succint.

esabol · 2023-09-24T20:27:27Z

doc/install.sgm

+   &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


Change "sources" to "source code". Native English speakers wouldn't use the plural in this context.

esabol · 2023-09-24T20:29:32Z

doc/install.sgm

+  </programlisting>
+
+  <para>
+   Compile the code. By default, &pgsphere; is compiled with the HEALPIX


Change "the HEALPIX" to "HEALPix" (no "the" and lowercase "ix").

esabol · 2023-09-24T20:30:06Z

doc/install.sgm

-            </para>
-            <programlisting>
+  </programlisting>
+  <para>or compile without HEALPIX support:</para>


Change "HEALPIX" to "HEALPix" here as well.

esabol · 2023-09-24T20:33:21Z

doc/install.sgm

+
+  <para>
+   Run regression tests optionally. If the &pgsphere; was compiled without
+   HEALPIX support, USE_HEALPIX=0 should be specified in make command line.


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".

esabol · 2023-09-24T20:34:42Z

doc/install.sgm

+  </programlisting>
+
+  <para>
+   Run regression tests optionally. If the &pgsphere; was compiled without


Remove "the" before "&pgsphere;".

esabol · 2023-09-24T20:37:21Z

doc/install.sgm

+  <para>
+   Install &pgsphere; files to the installation directories. The installation
+   directories are defined by &pg_config; utility. Superuser (root) access
+   rights may be required.


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."

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.

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.

@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!

esabol · 2023-09-24T20:40:10Z

doc/install.sgm

-                call:
-            </para>
-            <programlisting>
+  <para>To get the version of installed pgSphere software:</para>


Change "pgSphere" to "&pgsphere;" here?

esabol

One more change, please. Thank you!

esabol · 2023-09-27T15:15:29Z

doc/install.sgm

+  </programlisting>
+
+  <para>
+   Compile the code. By default, &pgsphere; is compiled with the &healpix;


Please remove "the" before "&healpix;" on this line. (Sorry I didn't notice this in my previous review.)

@esabol It's not a big deal. I've removed the article and rebased the branch. Thank you!

esabol

Looks good! Thank you!

vitcpp · 2023-09-28T15:22:34Z

Rebased

esabol suggested changes Sep 19, 2023

View reviewed changes

vitcpp force-pushed the doc-improve-2 branch 2 times, most recently from 6727a09 to d720dc6 Compare September 22, 2023 10:16

esabol suggested changes Sep 24, 2023

View reviewed changes

esabol suggested changes Sep 27, 2023

View reviewed changes

vitcpp force-pushed the doc-improve-2 branch from 8a03ed4 to f867318 Compare September 27, 2023 20:53

esabol approved these changes Sep 27, 2023

View reviewed changes

esabol mentioned this pull request Sep 27, 2023

Add GitHub Actions pipeline for pgSphere #69

Merged

vitcpp added 5 commits September 28, 2023 18:16

Remove the obsolete text from the doc

302c501

Fix some typos

8ebb3a9

Update Installation section

fea5f0d

Elaborate review comments from @esabol

be40da7

Removed unnecessary 'the' article

7195184

vitcpp force-pushed the doc-improve-2 branch from f867318 to 7195184 Compare September 28, 2023 15:17

esabol approved these changes Sep 28, 2023

View reviewed changes

vitcpp merged commit 764642b into postgrespro:master Sep 29, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the obsolete text from the doc #65

Remove the obsolete text from the doc #65

vitcpp commented Sep 19, 2023

esabol Sep 19, 2023

esabol Sep 19, 2023

esabol commented Sep 20, 2023

df7cb commented Sep 20, 2023

esabol commented Sep 20, 2023

vitcpp commented Sep 21, 2023

vitcpp commented Sep 21, 2023

df7cb commented Sep 21, 2023

vitcpp commented Sep 21, 2023

esabol commented Sep 21, 2023 •

edited

Loading

vitcpp commented Sep 21, 2023

esabol commented Sep 22, 2023

vitcpp commented Sep 22, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

esabol Sep 24, 2023

vitcpp Sep 26, 2023

esabol Sep 26, 2023

vitcpp Sep 27, 2023

esabol Sep 24, 2023

esabol left a comment

esabol Sep 27, 2023

vitcpp Sep 27, 2023

esabol left a comment

vitcpp commented Sep 28, 2023

Remove the obsolete text from the doc #65

Remove the obsolete text from the doc #65

Conversation

vitcpp commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol commented Sep 20, 2023

df7cb commented Sep 20, 2023

esabol commented Sep 20, 2023

vitcpp commented Sep 21, 2023

vitcpp commented Sep 21, 2023

df7cb commented Sep 21, 2023

vitcpp commented Sep 21, 2023

esabol commented Sep 21, 2023 • edited Loading

vitcpp commented Sep 21, 2023

esabol commented Sep 22, 2023

vitcpp commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esabol left a comment

Choose a reason for hiding this comment

vitcpp commented Sep 28, 2023

esabol commented Sep 21, 2023 •

edited

Loading