Skip to content
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

Fix build under mingw64 #123

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

vitcpp
Copy link
Contributor

@vitcpp vitcpp commented Apr 27, 2024

No description provided.

@vitcpp
Copy link
Contributor Author

vitcpp commented Apr 27, 2024

@robe2 This patch should fix your issue. Could you please check the compilation of this patch on your side? Thank you in advance.

@vitcpp vitcpp linked an issue Apr 27, 2024 that may be closed by this pull request
@esabol
Copy link
Contributor

esabol commented Apr 27, 2024

These changes are more extensive than I was expecting. I'm not sure they should be included in 1.5.1.

@vitcpp
Copy link
Contributor Author

vitcpp commented Apr 27, 2024

@esabol Actually, my changes are in sparse.y and in sscan.l. Other files (h/c) are auto generated from those two ones.

@@ -1,6 +1,6 @@
#line 2 "sscan.c"
#line 2 "src/sscan.c"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other changes which reference "src/somefile" don't look right to me. I suspect you autogenerated these files when you were in the directory above the "src" directory. Was that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @vitcpp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some times ago we moved all the sources into src subdirectory but the compilation is executed in the directory above. It is why we see these differences. I think, it should not be a problem, but, anyway, I will take a deeper look tomorrow. Sorry for the delay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if it's intentional and expected, then I'm fine with it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some more explanation.

There are two files sparse.y and sscan.l that are used to generate (src/sparse.h, src/sparse.c) and sscan.c accordingly. You may remove these generated sources and run make - these files will be generated again. These files are not required to be in the repository. I think, they were added to simplify the build process - lex/yacc tools are not required.

The following commands are used by make to generate these sources:
/usr/bin/flex -Psphere -osrc/sscan.c src/sscan.l yacc -d -p sphere_yy -o src/sparse.c src/sparse.y

Some times ago the sources were located in the pgsphere root directory. Then, we moved source files to src subdirectory. It seems, these files were not re-generated after that. Once we run flex/yacc from the pgsphere root directory, we add src/ prefix to the input file name. As the result, #line directive is changed. It may affect error messages when the compiler finds some build errors after that directive - file and line will be taken from this directive. I do not see any other ways it may affect the functionality somehow.

Some other changes occurred when I used newer versions of lex/yacc. As the result we see some changes in comments, produced by these tools in generated files (different tool versions or urls, some new explanations). These changes should not produce any problems.

There are some new token types in sparse.h file should be investigated:

  • YYEMPTY = -2
  • YYEOF = 0, /* "end of file" */
  • YYerror = 256, /* error */
  • YYUNDEF = 257, /* "invalid token" */

I'm not sure how they affect the parser functionality. I think, I should investigate it just to make sure everything is ok (tests are passed ok with these changes).

I haven't investigated the changes in sparse.c. I think there is no sense to do it. Because lex/yacc with different versions may produce different code.

My own changes - add TOK_ prefix to some token names (SIGN, INT, FLOAT, EULERAXIS) in input files sparse.y, sscan.l to fix the compilation.

In general, we may remove the generated files and ask user to install lex/yacc to build pgsphere.

Finally, I think to investigate how these new tokens affect the parser to make sure everything is ok.

@esabol
Copy link
Contributor

esabol commented Apr 28, 2024

@vitcpp wrote:

Actually, my changes are in sparse.y and in sscan.l. Other files (h/c) are auto generated from those two ones.

I realize that. It's still more changes than I'm comfortable with personally, but whatever. If you want to include it in 1.5.1, I'm fine with it.

@vitcpp
Copy link
Contributor Author

vitcpp commented Apr 28, 2024

@esabol Well, lets postpone this fix until the next release. Honestly, I'm not comfortable with these changes as well. Tomorrow I will create a new release.

Add TOK_ prefix to INT, FLOAT, SIGN token names to distinguist these
names from macro directives under mingw64.
@vitcpp
Copy link
Contributor Author

vitcpp commented May 20, 2024

I decided to propose a less invasive fix. There are two commits: first commit just regenerates parser files, second commit does the fix of token names. I used two commits to make differences more predictable. I used GNU Bison 3.4.2 to generate parser files.

@@ -4,5 +4,5 @@
#
#----------------------------------------------------------------------------

EXTENSION := pg_sphere
PGSPHERE_VERSION := 1.5.1
EXTENSION := pg_sphere
Copy link
Contributor

@esabol esabol May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whitespace change doesn't look right on GitHub, but I take it that it looks right in your editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esabol I just replaced spaces with tabs. It was ok for me because my editor used 4 spaces for tab. I will roll back this change. Thank you.

@vitcpp vitcpp merged commit 0607488 into postgrespro:master May 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiling under windows mingw64
2 participants