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

[octave] add new port #38243

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Apr 17, 2024

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.
@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 18, 2024
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Just first feedback.
Too early to decide on spending more time on this PR.

"description": "High-level interpreted language, primarily intended for numerical computations.",
"homepage": "https://octave.org/",
"license": "GPL-3.0-or-later",
"supports": "linux & !static",
Copy link
Contributor

Choose a reason for hiding this comment

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

This disables all vcpkg CI coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but I only succeeded to compile this port with vcpkg on dynamic ubuntu.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not allowed for the following reasons:

  1. Currently, the official Linux we support is statically built. Your expression will cause our CI to be unable to detect and run. triplets/x64-linux.cmake:
set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE static)

set(VCPKG_CMAKE_SYSTEM_NAME Linux)
  1. I found that the upstream supports windows & linux & osx. In this case, "support" should be consistent with the upstream. You can write the failed triplets into ci.baselin.txt:
    image
ports/octave/vcpkg.json Outdated Show resolved Hide resolved
@talregev
Copy link
Contributor Author

Just first feedback. Too early to decide on spending more time on this PR.

I admit that needed more work to compile this port on more triplets.

@MonicaLiu0311
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).
@MonicaLiu0311
Copy link
Contributor

Ping @talregev

@talregev
Copy link
Contributor Author

@MonicaLiu0311 Thank you for your review.
I will take a look and I will respond

@talregev talregev force-pushed the TalR/octave9 branch 2 times, most recently from 30a0cfa to 5e02f69 Compare May 30, 2024 17:44
@talregev
Copy link
Contributor Author

@talregev
Copy link
Contributor Author

@MonicaLiu0311 On the build system itself it say it cannot build with static: https://github.com/gnu-octave/octave/blob/3fedbfc38b3f06f1ade3db4b0f011191a18b9f92/configure.ac#L637

I will try to patch and remove this error and see what will happen.

@dg0yt
Copy link
Contributor

dg0yt commented May 30, 2024

After reading the issue mentioned in configure.ac, it seems that this port os a candidate for

vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)
@talregev talregev force-pushed the TalR/octave9 branch 2 times, most recently from 97810bf to 7d67b2f Compare June 1, 2024 08:05
@talregev
Copy link
Contributor Author

talregev commented Jun 1, 2024

Revert the port to be non static.
Wait all the ci that will fail, then I will add it to ci.baselin.txt

@MonicaLiu0311
Copy link
Contributor

@talregev Is there any new progress?

@talregev
Copy link
Contributor Author

for me, I finished develop this port.
It has only dynamic Linux working.
Should at least dynamic windows work?

If you want to review, go ahead, and let me know what you think.

@dg0yt
Copy link
Contributor

dg0yt commented Jul 10, 2024

There must be at least one CI configuration which builds the port.
And this is how it could be done: #38243 (comment).

@talregev
Copy link
Contributor Author

@dg0yt Do you want to try this? You can use my commits.

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!
3 participants