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

add grass-dev #3

Closed
wants to merge 5 commits into from
Closed

add grass-dev #3

wants to merge 5 commits into from

Conversation

ninsbl
Copy link
Contributor

@ninsbl ninsbl commented Sep 24, 2021

This PR adds a new grass-dev package that could be used for e.g nightly or weekly builds of the development version.
The code is based on the grass package. It is basically tested (meaning grass starts and runs a couple of tests succesfully).
Please review critically, esp. with regards to naming of the package, executable or app directory. @landam and @hellik, please have a look as well. I am open to any suggestion for changes. I would like to use the build recipe in CI. Therfore I added a CI variable to make it run as a github action. I hope that is acceptable...

The contained patch of the GRASS source I hope we can inetgrate upstreams at some point...

Let me know if you need something tested specifically, before integration/packaging...

Would be great to have a GRASS 8 preview packaged in the course of FOSS4G I would say...

cd ../$P-$V

if [ "$CI" ] ; then
P=$(cygpath -ua "C:/Program Files (x86)\Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/")
Copy link
Owner

Choose a reason for hiding this comment

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

P is supposed to contain the package name. What's wrong with following

        export VCPATH=$(
                vs2019env
                echo ${PATH//\/cygdrive/}
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your swift reply. For some reason the location of dumpbin did not get added to the path by vs2019env. Can have a second look at what actually happens...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still not 100% sure what happens here, maybe the PATH variable gets overwritten y subsequent calls of fetchenv...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so the problem is that vs2019env does not provide the path to dumpbin in GH actions. There, dumpbin seems to be moved to another directory (the one provided here).
I guess the path could be derived from the VCPATH, but that is for another PR... Anyway, this is now moved into the GRASS code-base (mklibs.sh)...

if [ "$CI" ] ; then
P=$(cygpath -ua "C:/Program Files (x86)\Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/")
sed -i "3 a export PATH=\"$P:$PATH\"" mswindows/osgeo4w/mklibs.sh
sed -i "s/dumpbin -exports/dumpbin \/EXPORTS/" mswindows/osgeo4w/mklibs.sh
Copy link
Owner

Choose a reason for hiding this comment

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

update the patch instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will update accordingly (also the variable name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into the package.sh patch in 3e8c9a8

branch=main

if [ "$CI" ] ; then
cd "$OSGEO4W_REP"
Copy link
Owner

@jef-n jef-n Sep 24, 2021

Choose a reason for hiding this comment

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

OSGEO4W_REP is supposed to be the directory where the package repository is (which is not neccessarilly the the base directory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I just use that as entry point in my github action where OSGEO4W_REP is set. Can use another variable name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variable renamed in 3e8c9a8

@ninsbl
Copy link
Contributor Author

ninsbl commented Sep 24, 2021

BTW, i tried to add PDAL support, that would be really nice to have. But because that is compiled with MSVC I really struggle to make that work. Any suggestion on how (or if) that can be achieved in a msys2 build / or how it should work would be very much appreciated!

@jef-n
Copy link
Owner

jef-n commented Sep 24, 2021

v.in.pdal uses C++ API. That won't work with different compilers involved.

@hellik
Copy link

hellik commented Sep 24, 2021

@jef-n
Copy link
Owner

jef-n commented Sep 24, 2021

@hellik
Copy link

hellik commented Sep 24, 2021

No, it's just the link to the C API for reference.

Not that GRASS is using it.

@hellik
Copy link

hellik commented Sep 24, 2021

No, it's just the link to the C API for reference.

Not that GRASS is using it.

@wenzeslaus any idea?

@ninsbl
Copy link
Contributor Author

ninsbl commented Sep 27, 2021

v.in.pdal uses C++ API. That won't work with different compilers involved.

OK, does that mean we either have to build GRASS with MSVC on Windows or build a (probably internal) version of pdal with g++?

Could using clang be an option that attempts to be compatible with both MSVC and GCC:
https://clang.llvm.org/docs/MSVCCompatibility.html

Or is any of thos options so unrealistic / unreliable that we rather have to accept the lack of PDAL support on MS Windows?

@wenzeslaus
Copy link

My intention was to use the C API, but at that time it was not really maintained (I don't know what is the situation now) and the recommendation I got from PDAL project people was to use C++ API, so that's what is used for r.in.pdal and v.in.pdal.

@ninsbl
Copy link
Contributor Author

ninsbl commented Oct 2, 2021

Based on the changes here I made a PR upstreams (OSGeo/grass#1921) that would make the patch obsolet.

@ninsbl
Copy link
Contributor Author

ninsbl commented Oct 10, 2021

Depends on: OSGeo/grass#1921

@ninsbl
Copy link
Contributor Author

ninsbl commented Oct 10, 2021

Or is any of thos options so unrealistic / unreliable that we rather have to accept the lack of PDAL support on MS Windows?

It seems the only viable option to get PDAL support for now is to build GRASS with MSVC. There are some early atempts:
OSGeo/grass#289
OSGeo/grass#348
But unfortunately, unfinished (it seems to be a bigger change)...

So here, we leave PDAL out for now...

@ninsbl
Copy link
Contributor Author

ninsbl commented Oct 26, 2021

With the patch merged upstreams, thic should be ready to merge. It builds nicely in my GH workflow...

@ninsbl ninsbl requested a review from jef-n October 26, 2021 20:38
@hellik
Copy link

hellik commented Oct 26, 2021

So here, we leave PDAL out for now...

Yes

@hellik
Copy link

hellik commented Oct 26, 2021

v.in.pdal uses C++ API. That won't work with different compilers involved.

OK, does that mean we either have to build GRASS with MSVC on Windows or build a (probably internal) version of pdal with g++?

Could using clang be an option that attempts to be compatible with both MSVC and GCC: https://clang.llvm.org/docs/MSVCCompatibility.html

Or is any of thos options so unrealistic / unreliable that we rather have to accept the lack of PDAL support on MS Windows?

It's unrealistic to switch to a MSVC build in the next future.

OTOH maybe a (sponsored) long term attempt for a such switch may be in the focus of the project

@ninsbl
Copy link
Contributor Author

ninsbl commented Jan 17, 2022

A gentle ping: RC2 was realeased for GRASS 8.0: https://github.com/OSGeo/grass/releases/tag/8.0.0RC2

It would be therefore really great to have a stable, ltr and development version for GRASS in OSGeo4W. Anything I or he GRASS GIS team could do to facilitate that?

Fully understand and appreciate that it needs to be included in a way that does not increase maintenance burden unreasonably...

@jef-n jef-n closed this in 5114f8a Feb 1, 2022
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.

4 participants