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

[BlingFire] Adding BlingFire Package #28304

Closed
wants to merge 10 commits into from

Conversation

TomasMorris
Copy link
Member

  • What does your PR fix?

Adds a BlingFire port

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Windows x64, Yes

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for blingfire have changed but the version was not updated
version: 0.1.8.1
old SHA: eaec4e8f6c06aab2b45efeb9390ad254d6bf26f6
new SHA: 116f5d9d79316a7186a821bfb0c02607cb4e4b8c
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions[bot]
github-actions bot previously approved these changes Dec 12, 2022
@JonLiu1993 JonLiu1993 changed the title Adding BlingFire Package [BlingFire] Adding BlingFire Package Dec 13, 2022
@JonLiu1993 JonLiu1993 self-assigned this Dec 13, 2022
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 13, 2022
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Dec 13, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 13, 2022
JonLiu1993
JonLiu1993 previously approved these changes Dec 14, 2022
@JonLiu1993
Copy link
Member

@TomasMorris, when I install this port on my local machine, I find no xxxconfig.cmake in package file but Upstream code have this code, could you please take a look?
image

INSTALL(
  TARGETS blingfiretokdll
  EXPORT blingfire-targets
  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
  INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
@TomasMorris
Copy link
Member Author

@TomasMorris, when I install this port on my local machine, I find no xxxconfig.cmake in package file but Upstream code have this code, could you please take a look? image

INSTALL(
  TARGETS blingfiretokdll
  EXPORT blingfire-targets
  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
  INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}

codebase didn't include a cmake config, I have updated BlingFire to include that and updated the commit in this port to match.

@JonLiu1993
Copy link
Member

JonLiu1993 commented Dec 20, 2022

@TomasMorris, when I tested the usage by a simple cmake project, I got this error.
Please take a look:
CMakeLists:

cmake_minimum_required (VERSION 3.8)

project(test)

find_package(blingfire CONFIG REQUIRED)
add_executable (main "CMakeProject42.cpp" "CMakeProject42.h")

target_link_libraries(main PRIVATE blingfiretokdll)

CMakeProject42.cpp:

// CMakeProject42.cpp : Defines the entry point for the application.
//

#include "CMakeProject42.h"
#include "blingfiretokdll.h"

using namespace std;

int main()
{
	cout << "Hello CMake." << endl;
	return 0;
}

CMakeProject42.h:

// CMakeProject42.h : Include file for standard system include files,
// or project specific include files.

#pragma once

#include <iostream>

// TODO: Reference additional headers your program requires here.

CMake Error at F:/Feature-test/bling/vcpkg/installed/x64-windows/share/blingfire/blingfire-config.cmake:1:
1> [CMake]   Parse error.  Expected a command name, got unquoted argument with text
1> [CMake]   "@PACKAGE_INIT@".
1> [CMake] Call Stack (most recent call first):
1> [CMake]   F:/Feature-test/bling/vcpkg/scripts/buildsystems/vcpkg.cmake:843 (_find_package)
1> [CMake]   CMakeProject42/CMakeLists.txt:10 (find_package)

@JonLiu1993
Copy link
Member

Pinging @TomasMorris for response.

@JonLiu1993
Copy link
Member

Convert this PR to draft since there is no progress. Please ping us if this PR is ready for review again.

@JonLiu1993 JonLiu1993 marked this pull request as draft March 13, 2023 08:10
@JonLiu1993
Copy link
Member

Closing this PR since it seems that no progress is being made. Please ping us to reopen if work is still being done.

@TomasMorris
Copy link
Member Author

Sorry for the break, took some time off. @JonLiu1993, would you mind reopening this as it should be ready for review and all the issues above were fixed.

@JonLiu1993
Copy link
Member

@TomasMorris, this PR cannot be reopened, thinking that the branch TomasMorris:master has been forced to push or no longer exists, could you please resubmit a 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants