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 rules_erlang 3.2.0 #101

Closed
wants to merge 9 commits into from
Closed

Conversation

pjk25
Copy link
Contributor

@pjk25 pjk25 commented Jun 13, 2022

meteorcloudy
meteorcloudy previously approved these changes Jun 13, 2022
@pjk25
Copy link
Contributor Author

pjk25 commented Jun 13, 2022

@meteorcloudy the change of build_targets seems to cause a surprising failure related to toolchain resolution:

(13:28:18) ERROR: /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/f17a9fe25e9beb64ab6d1873361ff593/external/@rules_erlang.3.1.0/tools/BUILD.bazel:8:15: While resolving toolchains for target @@rules_erlang.3.1.0//tools:erlang_headers: No matching toolchains found for types @@rules_erlang.3.1.0//tools:toolchain_type.
--
  | To debug, rerun with --toolchain_resolution_debug='@@rules_erlang.3.1.0//tools:toolchain_type'

The erlang_headers rule contains toolchains = [":toolchain_type"],, so I'm guessing this is a side effect of repo-mapping by bzlmod? Maybe toolchains = [str(Label("//tools:toolchain_type"))], might work, but seems like a bit of an odd workaround.

@meteorcloudy
Copy link
Member

/cc @Wyverald

@Wyverald
Copy link
Member

You probably ran into a breakage at Bazel@HEAD that causes toolchain resolution errors. I'm debugging it as we speak -- hopefully have something to share in the next hour. If not, I'll look at rolling back the change. Sorry for the inconvenience!

@Wyverald
Copy link
Member

Fix out at bazelbuild/bazel#15666

@pjk25
Copy link
Contributor Author

pjk25 commented Jun 14, 2022

Thanks @Wyverald . How do I know when the fix has propagated sufficiently to retest? You changes appear merged, but the error persists after I rebased this branch to re-trigger the checks.

@Wyverald
Copy link
Member

Sorry, this turned out to be much more involved. My fix only addressed a subset of the breakage. The full fix would require bazelbuild/bazel#14852 to be fixed, which will probably take another week or so. So I'm going to roll back the partial fixes to the previous delicate balance we had (which happened to work) for now. Will keep you updated here.

@Wyverald
Copy link
Member

The rollback has been submitted (bazelbuild/bazel@4ccc21f), but the postsubmit pipeline hasn't caught up due to a flaky test, so "last_green" doesn't include this change yet. You can follow the BuildKite page for the pipeline progress.

@pjk25
Copy link
Contributor Author

pjk25 commented Jun 15, 2022

Thanks for the updates @Wyverald

@pjk25 pjk25 force-pushed the rules_erlang-3.2.0 branch 2 times, most recently from 1bd8866 to 2219945 Compare June 16, 2022 13:16
@pjk25
Copy link
Contributor Author

pjk25 commented Jun 16, 2022

There seem to have been several green commits in the above pipeline, but the same error in the checks. What would I be looking for to determine the rollback on bazel has propagated sufficiently?

pjk25 added a commit to rabbitmq/rules_erlang that referenced this pull request Jun 16, 2022
@pjk25
Copy link
Contributor Author

pjk25 commented Jun 17, 2022

@Wyverald I've rebased this a couple of time and the checks still seem to fail. I wonder if I am running into a more complicated issue than originally thought. These checks passed when the build_target of the presubmit was //..., and it's not really clear to me why that triggers a different behavior (unless it was just coincidence that I created the PR prior to the breakage being merged to bazel master).

I am wondering if I ought to remove the

register_toolchains(
    ":erlang_toolchain_external",
)

from the WORKSPACE.bazel and use the --extra_toolchains flag. I was not able to get register_toolchains to work correctly as invoked by MODULE.bazel or via module extension. The bzlmod docs don't mention toolchains as far as I know, so it was trial and error on my part.

@Wyverald
Copy link
Member

w.r.t toolchains -- the module directive has a toolchains_to_register attribute (https://bazel.build/rules/lib/globals#module) which allows you to register toolchains.

Beyond that, yes apparently some things are still broken :( I'm still trying to debug what changed. Sorry about the breakage!

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 8, 2022

@Wyverald this is still failing even after a recent rebase. It looks like toolchain resolution still has issues in certain cases? I recall when the build_targets in the presubmit.yml were - '//...', things worked, but when updated to - '@rules_erlang//...' it started to fail.

@Wyverald
Copy link
Member

it looks like you simply don't have any toolchains registered. Could you try adding the line registry_toolchains(":erlang_toolchain_external") to your MODULE.bazel file?

@@ -24,3 +24,7 @@ use_repo(
"getopt_src",
"xref_runner_src",
)

register_toolchains(
":erlang_toolchain_external",
Copy link
Member

Choose a reason for hiding this comment

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

sorry for the churn -- you need to write "//:erlang_toolchain_external" here (with the //)

@Wyverald
Copy link
Member

I recall when the build_targets in the presubmit.yml were - '//...', things worked, but when updated to - '@rules_erlang//...' it started to fail.

When you wrote //... you were not actually testing anything. BCR presubmit works by creating a fake module that has nothing but a dependency on the module under test (in this case rules_erlang). So the main workspace literally just has an empty BUILD file. That's why nothing was failing.

Now your 3.2.0 version is passing (well, will be passing once we fix the toolchain label, I think), but the 3.1.0 version will stay erroneous unless you also fix the MODULE.bazel file. But even that might not be great -- ideally you'd fix the MODULE.bazel file upstream, instead of just in the BCR.

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 11, 2022

Independently I'm confident 3.1.0 was working, so I don't imagine there is much value in fixing the presubmit for something that has already been published. I'll fix it upstream since I already have 3.3.0 ready to go.

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 11, 2022

That does not seem to be sufficient, adding register_toolchains to the MODULE.bazel.

@Wyverald
Copy link
Member

That does not seem to be sufficient, adding register_toolchains to the MODULE.bazel.

Do you mean that adding register_toolchains to the MODULE.bazel file of 3.3.0 still doesn't work?

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 11, 2022

Do you mean that adding register_toolchains to the MODULE.bazel file of 3.3.0 still doesn't work?

I added it to the MODULE.bazel of this PR. I have rules_erlang@3.2.0 merged into our fork of BCR on a different branch, dev, and it's working fine with all of our projects.

Edit:
For a bit more context, since register_toolchains is not yet in bazel 5.2.0, we call it in the WORKSPACE.bazel instead.

@Wyverald
Copy link
Member

Calling register_toolchains in WORKSPACE only registers it when you're building from within rules_erlang. If you're a Bazel module that has a bazel_dep on rules_erlang, you won't have that toolchain registered. The only way to get the toolchain registration to propagate to dependents is to have rules_erlang call register_toolchains in its MODULE.bazel file, or have the dependants individually register that toolchain as well.

If it's working for all your projects, it has to be because your projects are registering the toolchain themselves too somewhere.

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 11, 2022

Ah, you're right. In some cases we are using the older toolchains_to_register in MODULE.bazel only.

As I recall we do not want unconditional registration for modules
depending on rules_erlang
@pjk25
Copy link
Contributor Author

pjk25 commented Aug 11, 2022

I remembered that we do not register this particular toolchain unconditionally, so it's purposefully only declared in the WORKSPACE. Then it finally sunk in regarding the way modules are tested as you described above. So, I added the toolchain registration to the presubmit.yml via build_flags. However that doesn't seem to work either.

@pjk25
Copy link
Contributor Author

pjk25 commented Aug 15, 2022

After a little more trial and error, I realized that previous checks must have relied on some undocumented behavior of the test infrastructure. The .bazelrc included in rules_erlang does not apply, so by adding additional build_flags to match the missing content, the build now proceeds. However, I've in turned discovered that a ~ in certain paths (which is now the present in the latest bazel) is not escaped properly. As such, a patch to rules_erlang is necessary. I will close this PR and open a new one with a newer rules_erlang containing this fix.

@pjk25 pjk25 closed this Aug 15, 2022
@pjk25 pjk25 deleted the rules_erlang-3.2.0 branch August 15, 2022 13:12
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.

3 participants