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 libxml2 2.9.12 #282

Closed
wants to merge 4 commits into from
Closed

Add libxml2 2.9.12 #282

wants to merge 4 commits into from

Conversation

jsharpe
Copy link
Contributor

@jsharpe jsharpe commented Nov 18, 2022

No description provided.

@fmeum
Copy link
Contributor

fmeum commented Nov 18, 2022

@jsharpe The module file is missing from the source archive. You can add it with a patch.

@jsharpe
Copy link
Contributor Author

jsharpe commented Nov 18, 2022

This was generated with the add_module.py tooling; it should be updated to add the module file patch automatically.

@meteorcloudy meteorcloudy added the awaiting user response Awaiting the PR author to take action label Dec 12, 2022
@jsharpe
Copy link
Contributor Author

jsharpe commented Dec 12, 2022

@meteorcloudy I stopped iterating on this because I don't have a local windows box setup to iterate the tests and having to wait for BCR maintainer approver for buildkite each time was becoming tedious. Are there some conditions under which we can persist the approval between CI runs on the same PR?

@jsharpe
Copy link
Contributor Author

jsharpe commented Dec 12, 2022

Alternatively I can just drop windows from the matrix?

@Wyverald
Copy link
Member

Are there some conditions under which we can persist the approval between CI runs on the same PR?

I think if a new commit is pushed to the PR which was previously approved by a registry maintainer, and the commit didn't touch the presubmit.yml file, we could persist the approval. I'm not sure how feasible that is (and whether that actually reduces the churn in your case?)

@meteorcloudy
Copy link
Member

meteorcloudy commented Dec 12, 2022

Or maybe we can add a label to indicate a PR is sent by a trusted contributor, then we bypass the BCR reviewer approval for CI run?

@alexeagle
Copy link
Contributor

GitHub actions has a nice default, where a contribution from someone with no commits on the project requires approval
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks
but once they've merged, they get the "contributor" status on the repo and approvals are no longer required for them.

@jsharpe
Copy link
Contributor Author

jsharpe commented Jan 18, 2023

@Wyverald @meteorcloudy what is the policy regarding having to have linux, macos and windows support working in presubmit.yml. I don't have bandwidth nor a windows setup to work out how to fix this on windows; can I just remove the windows platform from the test matrix?

@alexeagle
Copy link
Contributor

Yes, you can leave a TODO in the presubmit to enable windows in the matrix, like

# TODO(kormide): add "windows" when green

However I'm concerned that no one will ever pick up the TODO, so any module that depends on libxml won't work on Windows either. I think we should show this sort of compatibility warning on the registry.bazel.build to make it discoverable, though we'd have to do some parsing to figure it out.

@jsharpe
Copy link
Contributor Author

jsharpe commented Jan 18, 2023

Yes, you can leave a TODO in the presubmit to enable windows in the matrix, like

# TODO(kormide): add "windows" when green

However I'm concerned that no one will ever pick up the TODO, so any module that depends on libxml won't work on Windows either. I think we should show this sort of compatibility warning on the registry.bazel.build to make it discoverable, though we'd have to do some parsing to figure it out.

This is also a higher barrier to entry for module maintainers - they need to be fluent in building on all three major platforms in general to be able to publish high quality modules. I suspect that the number of people familiar with getting bazel builds working on windows is relatively low and may mean that generally windows becomes a second class citizen in the registry?

@alexeagle
Copy link
Contributor

If you're a rule author, you already had the bad news that you need to be able to debug Bazel problems on Windows. If you're a library author like libxml2 maintainers, you also had to add preprocessor definitions or something.

The problem here is that the BCR has accidentally become a C/C++ package manager, so we have contributors who are not the rule author nor the library author. libxml2 probably does work on Windows, but you're just the messenger and don't know why unistd.h isn't on the windows machine used for CI.

Windows is already a second class experience in Bazel but some policy here would be important to prevent that getting worse.

@fmeum
Copy link
Contributor

fmeum commented Jan 19, 2023

I see becoming a C/C++ package manager as more than an accident: If we don't at least centralize the effort to improve Windows support for Bazel projects, it will never become better. It's just the right kind of complex and niche that everyone will silently try to fix their issues themselves in hackish ways.

Maybe some companies that use Bazel on Windows could contribute a bit of funding to motivate module maintainers to figure out Windows builds?

In this particular case, Windows support may require adding libiconv first (https://github.com/conan-io/conan-center-index/blob/master/recipes/libiconv/all/conanfile.py).

@meteorcloudy
Copy link
Member

what is the policy regarding having to have linux, macos and windows support

We don't really have a policy for this. I think it's fine to remove windows for now and leave a TODO there. But I think we do need a better way to indicate which modules work on which platforms.

@Wyverald
Copy link
Member

Hello! I'm doing some routine cleanup of stale PRs. If you're still working on this and planning to make progress soon, please let me know and we can re-open it.

@Wyverald Wyverald closed this Mar 21, 2024
This was referenced Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user response Awaiting the PR author to take action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants