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

[ncurses] Add file(MAKE_DIRECTORY) to create the missing directories #26690

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

Cheney-W
Copy link
Contributor

@Cheney-W Cheney-W commented Sep 5, 2022

Describe the pull request

  "--with-pkg-config-libdir=${CURRENT_INSTALLED_DIR}/debug/lib/pkgconfig"
  "--with-pkg-config-libdir=${CURRENT_INSTALLED_DIR}/lib/pkgconfig"

If this port install in a new vcpkg, it will failed with following error because there is no pkgconfig folder.

  configure: error: expected a pathname, not ""

For fixing this issue, add file(MAKE_DIRECTORY) to create the missing directories.

No feature need to test.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Sep 5, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 5, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Sep 5, 2022

Does it really need a pkgconf dependency, or does it just need those directories to exist?

@Cheney-W Cheney-W marked this pull request as ready for review September 6, 2022 05:46
@Cheney-W
Copy link
Contributor Author

Cheney-W commented Sep 6, 2022

No, it doesn't really require a pkgconf dependency, it just needs those directories to exist.

I think it is more convenient to add dependencies than adding the operation of creating those directories in portfile.cmake, pkgconf installation does not take time, and there is no problem of directory creation failure caused by certain permissions.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 6, 2022

Convenience over correctness? I see.

PS: IIRC the purpose of these lines is to not pick up system libs. A possible correct solution might be pointing to an empty temporary directory in the buildtrees.

@Cheney-W
Copy link
Contributor Author

Cheney-W commented Sep 6, 2022

@dg0yt Thanks for your comment.
What do you think about this change?

file(MAKE_DIRECTORY "${CURRENT_BUILDTREES_DIR}/temp/debug/lib/pkgconfig")
file(MAKE_DIRECTORY "${CURRENT_BUILDTREES_DIR}/temp/lib/pkgconfig")

set(OPTIONS_DEBUG
    "--with-pkg-config-libdir=${CURRENT_BUILDTREES_DIR}/temp/debug/lib/pkgconfig"
    --with-debug
    --without-normal
)
set(OPTIONS_RELEASE
    "--with-pkg-config-libdir=${CURRENT_BUILDTREES_DIR}/temp/lib/pkgconfig"
    --without-debug
    --with-normal
)

I tested it, ncurses:x64-linux can be installed successfully.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 6, 2022

What do you think about this change?

Yes, this is what I meant. However,

I tested it, ncurses:x64-linux can be installed successfully.

Does it still install the pc files to the right directories?

@Cheney-W
Copy link
Contributor Author

Cheney-W commented Sep 6, 2022

No. If use a temporary directory under buildtrees, the pc file will not be installed.
I tested it, if I use ${CURRENT_INSTALLED_DIR}/..., the pc file will be installed in packages folder and installed folder.

./installed/x64-linux/lib/pkgconfig/panel.pc
./installed/x64-linux/lib/pkgconfig/menu.pc
./installed/x64-linux/lib/pkgconfig/ncurses.pc
./installed/x64-linux/lib/pkgconfig/form.pc
./installed/x64-linux/lib/pkgconfig/ncurses++.pc
./installed/x64-linux/debug/lib/pkgconfig/panel_g.pc
./installed/x64-linux/debug/lib/pkgconfig/ncurses_g.pc
./installed/x64-linux/debug/lib/pkgconfig/ncurses++_g.pc
./installed/x64-linux/debug/lib/pkgconfig/form_g.pc
./installed/x64-linux/debug/lib/pkgconfig/menu_g.pc
./packages/ncurses_x64-linux/lib/pkgconfig/panel.pc
./packages/ncurses_x64-linux/lib/pkgconfig/menu.pc
./packages/ncurses_x64-linux/lib/pkgconfig/ncurses.pc
./packages/ncurses_x64-linux/lib/pkgconfig/form.pc
./packages/ncurses_x64-linux/lib/pkgconfig/ncurses++.pc
./packages/ncurses_x64-linux/debug/lib/pkgconfig/panel_g.pc
./packages/ncurses_x64-linux/debug/lib/pkgconfig/ncurses_g.pc
./packages/ncurses_x64-linux/debug/lib/pkgconfig/ncurses++_g.pc
./packages/ncurses_x64-linux/debug/lib/pkgconfig/form_g.pc
./packages/ncurses_x64-linux/debug/lib/pkgconfig/menu_g.pc
./buildtrees/ncurses/x64-linux-rel/misc/panel.pc
./buildtrees/ncurses/x64-linux-rel/misc/menu.pc
./buildtrees/ncurses/x64-linux-rel/misc/ncurses.pc
./buildtrees/ncurses/x64-linux-rel/misc/form.pc
./buildtrees/ncurses/x64-linux-rel/misc/ncurses++.pc
./buildtrees/ncurses/x64-linux-dbg/misc/panel_g.pc
./buildtrees/ncurses/x64-linux-dbg/misc/ncurses_g.pc
./buildtrees/ncurses/x64-linux-dbg/misc/ncurses++_g.pc
./buildtrees/ncurses/x64-linux-dbg/misc/form_g.pc
./buildtrees/ncurses/x64-linux-dbg/misc/menu_g.pc

But if I use ${CURRENT_PACKAGES_DIR}/..., the pc file will not be installed, same with the temporary directory under buildtrees.

@Cheney-W Cheney-W changed the title [ncurses] Add pkgconf as the dependence of ncurses [ncurses] Add file(MAKE_DIRECTORY) to create the missing directories Sep 6, 2022
@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Sep 7, 2022
@dan-shaw dan-shaw merged commit 0c1a8fe into microsoft:master Sep 8, 2022
@Cheney-W Cheney-W deleted the Dev/Cheney/26677 branch October 9, 2022 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ncurses:x64-linux] build failure
4 participants