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

Port upstream libgeos 3.11.2 #1

Closed
wants to merge 2 commits into from

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jul 25, 2023

GEOS is a C/C++ library for GIS computational geometry.

Due to requiring a tree build, depends on unikraft/unikraft#1001.

Part of work on porting the Shapely Python 3 extension.

@razvand razvand self-assigned this Aug 7, 2023
@razvand razvand added the enhancement New feature or request label Aug 7, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Few more comments.
Besides that, If I build with the unittests as main, I get multiple definitions of main:

/usr/bin/ld: workdir/apps/helloworld/build/libgeos_test.o: in function `main':
workdir/apps/helloworld/build/libgeos/origin/geos-3.11.2/tests/unit/geos_unit.cpp:57: multiple definition of `main';

Same with the cli main

/usr/bin/ld: workdir/apps/helloworld/build/libgeos.o: in function `main':
workdir/apps/helloworld/build/libgeos/origin/geos-3.11.2/util/geosop/GeosOp.cpp:54: multiple definition of `main'; 

README.md Outdated
Comment on lines 1 to 8
libGEOS for Unikraft
========================

This is a port of [GEOS](https://libgeos.org/) to Unikraft.

Please refer to the `README.md` as well as the documentation in the
`doc/` subdirectory of the main unikraft repository for further
information.
Copy link
Member

Choose a reason for hiding this comment

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

Use md syntax instead of rst. Use one line per sentence and one sentence per line.
Also do not reference the old doc/ subdirectory, send people to the Unikraft website instead.

Config.uk Outdated
Comment on lines 9 to 15
config LIBGEOS_CLI
bool "Provide the GeosOp command-line interface as main"
default n

config LIBGEOS_UNITTEST
bool "Provide the libgeos unit tests as main"
default n
Copy link
Member

Choose a reason for hiding this comment

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

These look like they should be mutually exclusive, since they both provide the main function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, will change to a prompt.

Comment on lines +4 to +5
select LIBCXX
select CXX_THREADS
Copy link
Member

Choose a reason for hiding this comment

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

Is libcxx the only library this depends on? If not, we should add the rest here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, modulo the dependencies of libcxx itself.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks good now @andreittr.
I suppose the tests are known to fail, right?

capi::GEOSBoundary: .
capi::GEOSBuffer: ....................
capi::GEOSBuildArea: .
capi::GEOSCAPIDefines: ...
capi::GEOSClipByRect: .............[    0.381145] CRIT: [libmusl] <abort.c @    7> abort called

CONTRIBUTING.md Outdated
Comment on lines 1 to 4
Contributing to Unikraft
=======================

Please refer to the `CONTRIBUTING.md` file in the main Unikraft repository.
Copy link
Member

Choose a reason for hiding this comment

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

Also use md format here instead of rst, and send people to the contributing section on the website (https://unikraft.org/docs/contributing/)

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Files under `include/` are generated by the upstream build process from
templates. We include these files verbatim for the unikraft build.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

andreittr commented Aug 7, 2023

I suppose the tests are known to fail, right?

Yeah... I remember GEOSClipByRect aborting out, but haven't really had the time to look into it during porting.
On the bright side, consuming software like Shapely doesn't seem to trigger this condition, so it's not critical.
Nevertheless, it'll probably crop back up during more testing of downstream packages, so I expect I'll need to fix it eventually.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thanks.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

All good also on my side. Thanks @andreittr

Reviewed-by: Radu Nichita radunichita99@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
Files under `include/` are generated by the upstream build process from
templates. We include these files verbatim for the unikraft build.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #1
@andreittr andreittr deleted the ttr/up-3-11-2 branch August 10, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

5 participants