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

[taskflow] Update cpp-taskflow 2.2.0 to taskflow 2.6.0 #13140

Merged

Conversation

mfornace
Copy link
Contributor

Describe the pull request

Bug fix to previous pull request updating cpp-taskflow to 2.5.0.

  • What does your PR fix? Fixes #

Library name is now changed to "taskflow." Target can be found correctly.

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

All of them, none have been updated.

Yes

@BillyONeal
Copy link
Member

Unfortunately this breaks vcpkg upgrade.... we might want to keep it using the old name in the ports tree. Will ask other maintainers about it.

Also you've got some merge conflicts :)

@mfornace
Copy link
Contributor Author

mfornace commented Aug 25, 2020

@BillyONeal OK thanks, I fixed the conflicts (didn't see them the first time). CI passed with the new name change to Taskflow.

I then added a patch to configure the project as Cpp-Taskflow (just had to change the PROJECT() call pretty much). Then the config file names are appropriate and it can stay as cpp-taskflow. Not that painful...I guess.

Not sure what is best in terms of renaming the package or not; I guess both could work for now. It is a permanent name change mentioned on the Taskflow github FWIW. ¯\_(ツ)_/¯

Edit: I also don't know what the CI errors on the latest commit mean at all...

Cannot find path 'C:\a\1\a\xml-results\x64-windows.xml' because it does not exist

@JackBoosY
Copy link
Contributor

Maybe we need to add a vcpkg-cmake-wrapper.cmake to solve this different. And for the msvc project, we can't do anything.

@JackBoosY JackBoosY linked an issue Aug 26, 2020 that may be closed by this pull request
@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please update the version info. See documentation.

@mfornace
Copy link
Contributor Author

@JackBoosY I updated to 2.5.0-1, it still works locally as well for me to link to the Cpp-Taskflow::Cpp-Taskflow target. Not sure what you decided about changing the port name but it seems OK to me as is, for now.

@BillyONeal
Copy link
Member

Not sure what you decided about changing the port name but it seems OK to me as is, for now.

We haven't discussed it yet.

I suspect we're either going to want a port left with the old name that depends on the current name, or we are going to not want to change the name at all.

To clarify, the scenario this breaks is:

:: some time a year ago
.\vcpkg.exe install cpp-taskflow
:: today
git pull
.\vcpkg upgrade --no-dry-run

@ras0219
Copy link
Contributor

ras0219 commented Sep 1, 2020

Renaming the library sounds good to me, plus having a forwarding port for some amount of time (the description should make it clear that it's deprecated).

@JackBoosY JackBoosY added category:port-update The issue is with a library, which is requesting update new revision and removed requires:discussion labels Sep 1, 2020
@BillyONeal
Copy link
Member

@mfornace OK, up to you. Can you indicate which of these you'd prefer:

  • Leave the port named cpp-taskflow; I see you have since updated your changes to do this
  • Rename the port to taskflow and add a new cpp-taskflow that does a message(WARNING "The port cpp-taskflow has been replaced with taskflow.") and a comment therein saying we'll remove it in ~6 months.

Thanks!

@remz1337
Copy link
Contributor

remz1337 commented Sep 3, 2020

@JackBoosY is there anything pending to approve this PR?
Thanks

@BillyONeal
Copy link
Member

@remz1337 I wanted to give @mfornace the opportunity to pursue the alias solution if they desired since we had landed on that being OK; #13140 (comment)

If we get no response by... let's say tomorrow, we'll probably merge as is.

@mfornace
Copy link
Contributor Author

mfornace commented Sep 3, 2020

Sorry, I lost track of this issue. I suppose I have no strong feeling on renaming the port... it is not a repo I am involved in, I should say. It seems for sure easier for us to leave the port as is (cpp-taskflow). The problem is anyone migrating (like me) to using vcpkg with Taskflow would have to change their CMake files for the different target name that is exported, so that's not a great solution.

To pursue the other solution, would we just include the current master version of cpp-taskflow with a warning, and then include the taskflow port I originally submitted as the PR?

If you agree with the latter solution, I can:

  • roll back the PR to leaving cpp-taskflow except for including message(WARNING "The port cpp-taskflow has been replaced with taskflow.")
  • roll back the PR to my original commit which just exports Taskflow::Taskflow, except now that content goes in the taskflow folder

And sorry for the dumb question, but how do you mark the change as resolved? :D

@JackBoosY
Copy link
Contributor

@mfornace git reset <COMMIT_ID>

Copy link
Contributor Author

@mfornace mfornace left a comment

Choose a reason for hiding this comment

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

Edited to 2.5.0-1

@mfornace
Copy link
Contributor Author

mfornace commented Sep 4, 2020

Sorry, I'm a bit clueless on the vcpkg PR process, but here's what I did:

  • Edited the cpp-taskflow port back to how it was before the whole PR, then updated to 2.4.0 (the most recent non-renamed version). This prevents the port from breaking on OS X, which was my original purpose in all this T_T (this is just a bug in cpp-taskflow's cmake at that time). Then I patched it a bit to disable building the examples since it is header-only.
  • Made a new taskflow port for new version 2.6.0 (released since PR was started), which fixes some of the 2.5.0 issues. I fixed the installation of the CMake files and added the minor MSVC patch.

From what I can tell, the CI fails because cpp-taskflow and taskflow install the same files (e.g. taskflow/**.h in both cases). I tested both locally and they worked in installation and linking to a dummy project (but they can't be installed at the same time).

I would appreciate some help on how to arrange the port(s) etc so that CI can pass correctly.

@BillyONeal
Copy link
Member

Unfortunately your changes here downgrade the cpp-taskflow port which is likely the cause of the failures here. Since you wanted the 'alias' solution I pushed changes that do that.

@BillyONeal BillyONeal changed the title Update cpp-taskflow 2.2.0 to taskflow 2.5.0 [taskflow] Update cpp-taskflow 2.2.0 to taskflow 2.6.0 Sep 7, 2020
ports/taskflow/fix-compiler-error.patch Outdated Show resolved Hide resolved
@mfornace
Copy link
Contributor Author

I removed the compiler patch file and edited the port to use vcpkg_fixup_cmake_targets.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 11, 2020
@ras0219-msft ras0219-msft merged commit 946fa30 into microsoft:master Sep 11, 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:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cpp-taskflow] error with latest update to 2.5.0
6 participants