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

Add --disable-icu flag for iconv backend #3855

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

daravi
Copy link
Contributor

@daravi daravi commented Dec 11, 2020

Specify library name and version: boost/all

  • [✓] I've read the guidelines for contributing.
  • [✓] I've followed the PEP8 style guides for Python code in the recipes.
  • [✓] I've used the latest Conan client version.
  • [✓] I've tried at least one configuration locally with the
    conan-center hook activated.

I'm not sure if this is a bug in Boost's documentation, its build system, or the recipe. But building without this flag I see that my boost binaries are linked with my system icu.

@ghost
Copy link

ghost commented Dec 11, 2020

@ghost ghost mentioned this pull request Dec 11, 2020
7 tasks
@prince-chrismc

This comment has been minimized.

@madebr
Copy link
Contributor

madebr commented Dec 24, 2020

@daravi
Is this problem still present with the new boost recipe, pushed to master today?

@SSE4 SSE4 closed this Jan 23, 2021
@SSE4 SSE4 reopened this Jan 23, 2021
@SSE4 SSE4 requested a review from uilianries January 23, 2021 16:45
@conan-center-bot
Copy link
Collaborator

All green in build 3 (ed49a80faf07185691267f3e15957a64b504c3e2)! 😊

@ghost ghost mentioned this pull request Jan 25, 2021
4 tasks
@conan-center-bot
Copy link
Collaborator

All green in build 4 (ed49a80faf07185691267f3e15957a64b504c3e2)! 😊

@prince-chrismc
Copy link
Contributor

It's another broken CI state 😖

Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

Something happened in this PR that generated more packages than usual. I am blocking the merge until we figure it out

@danimtb
Copy link
Member

danimtb commented Jan 27, 2021

The CI generated 2 different recipe revisions due to different job executions that took a different master state as the target branch. I have pushed an empty commit to regenerate the packages

@conan-center-bot
Copy link
Collaborator

All green in build 5 (a4a6b050678e6214ed79586b568b95fe0eedeb9d)! 😊

@prince-chrismc
Copy link
Contributor

The bot (mine too) seems to be incorrectly counting the reviews ... some were not dismissed but are not counted

@SSE4 SSE4 added the infrastructure Waiting on tools or services belonging to the infra label Jan 29, 2021
@SSE4
Copy link
Contributor

SSE4 commented Jan 29, 2021

@danimtb @jgsogo what is preventing it from merging?

@jgsogo
Copy link
Contributor

jgsogo commented Jan 29, 2021

Hi! The bot says that @danimtb is blocking it... but he has approved the PR (after the previous block) and I can see the right (expected) data via API (https://api.github.com/repos/conan-io/conan-center-index/pulls/3855/reviews). I'll debug the algorithm to see what is happening here.

Thanks for raising the hand! 👍

@jgsogo
Copy link
Contributor

jgsogo commented Jan 29, 2021

Sorry, I was wrong. The problem here is that the latest commit (a4a6b050678e6214ed79586b568b95fe0eedeb9d) has only be approved by @danimtb and @prince-chrismc, we need at least one more positive review.

Algorithm is working fine, but we are not taking into account empty commits (wish it was a way to get this information using Github API).

@jgsogo jgsogo removed the infrastructure Waiting on tools or services belonging to the infra label Jan 29, 2021
@jgsogo jgsogo requested a review from SSE4 January 29, 2021 09:40
@conan-center-bot conan-center-bot merged commit a9e2407 into conan-io:master Jan 29, 2021
@prince-chrismc
Copy link
Contributor

@jgsogo I fixed the issue in my bot https://github.com/prince-chrismc/conan-center-index-pending-review/blob/main/pending_review/pull_request.go#L106-L118

I count valid approvals regardless if they are on the head commit or not... this way it matches the github API more closely

It seems the API handles the DISMISSED case well

@jgsogo
Copy link
Contributor

jgsogo commented Jan 29, 2021

Hi, @prince-chrismc! Thanks for pointing it out. I'm not confident enough (1) and that is the reason why we are checking that the reviews are associated with the latest commit SHA.

To implement our logic we would need:

  • every APPROVED to be transformed into DISMISSED if a new commit modifies the sources.
  • but CHANGES_REQUESTED state should remain even if there are new commits

(1) it clearly means that we haven't tested it enough. 🙈

@prince-chrismc
Copy link
Contributor

The UI still shows a ✔️ #3855 (review)

Though the API reports it as a comment now

{
    "id": 550510346,
    "node_id": "MDE3OlB1bGxSZXF1ZXN0UmV2aWV3NTUwNTEwMzQ2",
    "user": {
      "login": "prince-chrismc",
      "id": 16867443,
      // ...
    },
    "body": "",
    "state": "COMMENTED",
    "html_url": "https://github.com/conan-io/conan-center-index/pull/3855#pullrequestreview-550510346",
    "pull_request_url": "https://api.github.com/repos/conan-io/conan-center-index/pulls/3855",
    "author_association": "CONTRIBUTOR",
    "_links": {
      "html": {
        "href": "https://github.com/conan-io/conan-center-index/pull/3855#pullrequestreview-550510346"
      },
      "pull_request": {
        "href": "https://api.github.com/repos/conan-io/conan-center-index/pulls/3855"
      }
    },
    "submitted_at": "2020-12-11T19:17:24Z",
    "commit_id": "ed49a80faf07185691267f3e15957a64b504c3e2"
  },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants