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

[triangle] Add new port #13322

Merged
merged 18 commits into from
Sep 15, 2020
Merged

[triangle] Add new port #13322

merged 18 commits into from
Sep 15, 2020

Conversation

n4z4m3
Copy link
Contributor

@n4z4m3 n4z4m3 commented Sep 3, 2020

Add the libigl-triangle port as per Issue #12564
Fixes #12564

@NancyLi1013 NancyLi1013 self-assigned this Sep 3, 2020
ports/libigl-triangle/CONTROL Outdated Show resolved Hide resolved
ports/libigl-triangle/copyright Outdated Show resolved Hide resolved
ports/libigl-triangle/portfile.cmake Outdated Show resolved Hide resolved
ports/libigl-triangle/portfile.cmake Outdated Show resolved Hide resolved
ports/libigl-triangle/portfile.cmake Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels Sep 3, 2020
@NancyLi1013 NancyLi1013 changed the title New Port : libigl-triangle [libigl-triangle] Add new port Sep 3, 2020
n4z4m3 and others added 5 commits September 2, 2020 21:16
Unnecessary Port-Version

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Unnecessary inclusion of vcpkg_common_functions

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Use README from source as the copyright file

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Can you please use official sources and not unofficial repository?
http://www.netlib.org/voronoi/triangle.zip

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Also, you install the header both in the CMakeLists and in the portfile. One should be enough :)

ports/libigl-triangle/CONTROL Outdated Show resolved Hide resolved
ports/libigl-triangle/portfile.cmake Outdated Show resolved Hide resolved
ports/libigl-triangle/portfile.cmake Outdated Show resolved Hide resolved
@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Final note: can you please also provide CMake targets, to link to triangle (prefixed with unofficial is ok if you prefer)

@NancyLi1013
Copy link
Contributor

Can you please use official sources and not unofficial repository?
http://www.netlib.org/voronoi/triangle.zip

It seems the repository is not unofficial repository. I noticed that libigl also used https://github.com/libigl/libigl.
But I'm also not sure about this.

@BillyONeal
Could you please help review this point? Also for the port name, I don't know if libigl-triangle is suitable. Or we might rename it as triangle.

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

It seems the repository is not unofficial repository

It is. I am also a user of triangle :)
Please look at http://www.cs.cmu.edu/~quake/triangle.html for the official homepage and official download link

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

It is really the same sources, nothing looks like might warrant even a libigl-triangle duplicate here

n4z4m3 and others added 2 commits September 3, 2020 08:37
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 3, 2020

Can you please use official sources and not unofficial repository?
http://www.netlib.org/voronoi/triangle.zip

I would like to. The reason I did not is because this is actually not a true mirror of the original sources. It has modifications to the source code that allow it to build as a library and also to support 64-bit architecture. However, I think I should be able to add these changes using patches. I will have to learn how to do the patching. Hopefully the documentation at https://vcpkg.readthedocs.io/en/latest/examples/patching will help.

Also, this should allow me to change the port name to just triangle.

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

I think I should be able to add these changes using patches. I will have to learn how to do the patching. Hopefully the documentation at https://vcpkg.readthedocs.io/en/latest/examples/patching will help.

yes please.
You will receive support in case of necessity

@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 3, 2020

Final note: can you please also provide CMake targets, to link to triangle (prefixed with unofficial is ok if you prefer)

I would like to, but I am pretty new to both creating vcpkg ports (which is probably pretty obvious by now) and CMake. So I don't really know how to do this. But I will try to figure it out. If you or somebody else can help that would be great. Even just pointing to a good example that I can copy from would be helpful.

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

I would like to, but I am pretty new to both creating vcpkg ports (which is probably pretty obvious by now) and CMake. So I don't really know how to do this. But I will try to figure it out. If you or somebody else can help that would be great. Even just pointing to a good example that I can copy from would be helpful.

I wrote many CMakeLists.txt for vcpkg. Look for example to ports/vlfeat for an example of a library that installs header files and provides targets for end users

@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Yeah right direction! 👆🏻

@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 3, 2020

I believe I have implemented all suggested changes.

I really appreciate all the suggestions. You are all awesome.

Note that there is one more change request which I believe is not going to be made. How do I close that? Or is that something that the person who made the request must do?

@n4z4m3 n4z4m3 changed the title [libigl-triangle] Add new port [triangle] Add new port Sep 3, 2020
@cenit
Copy link
Contributor

cenit commented Sep 3, 2020

Would you mind creating the triangle.exe target and move it to tools? That would make this a fully complete port (and in return you would also learn how to deal with executable tools in vcpkg ;) )

@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 3, 2020

Would you mind creating the triangle.exe target and move it to tools? That would make this a fully complete port (and in return you would also learn how to deal with executable tools in vcpkg ;) )

Yes. This has been done now.

@NancyLi1013
Copy link
Contributor

The regressions on x64-linux:

4/5] : && /usr/bin/cc -fPIC -g  -rdynamic CMakeFiles/triangle.dir/triangle.c.o  -o triangle   && :
FAILED: triangle 
: && /usr/bin/cc -fPIC -g  -rdynamic CMakeFiles/triangle.dir/triangle.c.o  -o triangle   && :
CMakeFiles/triangle.dir/triangle.c.o: In function `parsecommandline':
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:3527: undefined reference to `cos'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:3531: undefined reference to `sqrt'
CMakeFiles/triangle.dir/triangle.c.o: In function `circletop':
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:10529: undefined reference to `sqrt'
CMakeFiles/triangle.dir/triangle.c.o: In function `splitencsegs':
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:13362: undefined reference to `sqrt'
CMakeFiles/triangle.dir/triangle.c.o: In function `quality_statistics':
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15392: undefined reference to `cos'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15501: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15502: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15503: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15504: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15510: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15510: undefined reference to `acos'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15516: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15516: undefined reference to `acos'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15518: undefined reference to `sqrt'
/mnt/vcpkg-ci/buildtrees/triangle/src/triangle-84d2cb038f.clean/triangle.c:15518: undefined reference to `acos'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

Could you please look into this?

@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 4, 2020

The regressions on x64-linux:

Could you please look into this?

I believe I have fixed this. However, the checks are not re-running. Is that expected?

@cenit
Copy link
Contributor

cenit commented Sep 7, 2020

Try rebasing on master to re-trigger CI checks

In any case, thanks for applying all suggestions. Port looks very fine for me :)

@n4z4m3
Copy link
Contributor Author

n4z4m3 commented Sep 7, 2020

Try rebasing on master to re-trigger CI checks

In any case, thanks for applying all suggestions. Port looks very fine for me :)

Is this rebasing something I need to do? Because I have never had to do a rebase before and I am worried about messing something up.

@NancyLi1013
Copy link
Contributor

I have merged master to re-trigger CI checks. Let's see the new check results.

scripts/ci.baseline.txt Outdated Show resolved Hide resolved
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 8, 2020
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Other than my one comment, LGTM.

ports/triangle/enable_64bit_architecture.patch Outdated Show resolved Hide resolved
Nathan Mercer added 2 commits September 10, 2020 07:54
Also, define FLOAT and VOID in the header so the user of the library does not have to define them
…be float or double. Also make the define for VOID be void rather than replacing all VOID with void in order to reduce the patch size.
@ras0219-msft ras0219-msft merged commit 47c0b1c into microsoft:master Sep 15, 2020
@ras0219-msft
Copy link
Contributor

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] libigl-triangle
4 participants