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

[email protected] #3430

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

publish-to-bcr-bot[bot]
Copy link
Contributor

@bazel-io
Copy link
Member

Hello @chickenandpork, modules you maintain (rules_synology) have been updated in this PR. Please review the changes.

@chickenandpork
Copy link
Contributor

Unsure if I should just post the next version or wait for this PR to confirm whether the presubmit has become acceptable

@fmeum fmeum added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Dec 28, 2024
fmeum
fmeum previously approved these changes Dec 28, 2024
platform:
- ubuntu2204
- macos
bazel: [7.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bazel: [7.x]
bazel: [7.x, 8.x]

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for suggestions — I’ll patch into this rev

@chickenandpork
Copy link
Contributor

I’ll take a look in ~12 hours

@bazel-io
Copy link
Member

Hello @chickenandpork, modules you maintain (rules_synology) have been updated in this PR. Please review the changes.

@bazel-io bazel-io dismissed fmeum’s stale review December 30, 2024 21:44

Require module maintainers' approval for newly pushed changes.

@chickenandpork
Copy link
Contributor

Hey @fmeum having trouble still repro-ing the failures:

git checkout v0.2.0 &&  \
USE_BAZEL_VERSION=8.0.0 bazel --nohome_rc \
  --host_jvm_args=-Djava.net.preferIPv6Addresses=false \
  build \
  --show_progress_rate_limit=5 --curses=yes --color=yes --terminal_columns=143 \
  --show_timestamps --verbose_failures --jobs=8 --announce_rc --experimental_repository_cache_hardlinks \
  --disk_cache= --jvmopt=-Djava.net.preferIPv6Addresses \
  --remote_max_connections=200 --remote_download_toplevel \
  --test_env=HOME --test_env=BAZELISK_USER_AGENT --test_env=USE_BAZEL_VERSION \
  --test_env=COURSIER_OPTS --test_env=JAVA_TOOL_OPTIONS --test_env=SSL_CERT_FILE \
  -- @rules_synology//... -@rules_synology//examples/...

Builds fine, so there are other options at play here. Note that my bazel is a softlink to basilisk so should use the USE_BAZEL_VERSION variable, and I've removed the .bazelversion to be certain.

Do we have a way to repro these tests more accurately ?

Copy link
Contributor

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

The failure comes up in the validation step (https://buildkite.com/bazel/bcr-presubmit/builds/9769#01941986-7ca5-477a-a4de-c32f9a17764f). Looks like your source archive is missing the MODULE.bazel file.

@chickenandpork
Copy link
Contributor

chickenandpork commented Dec 31, 2024

The failure comes up in the validation step (https://buildkite.com/bazel/bcr-presubmit/builds/9769#01941986-7ca5-477a-a4de-c32f9a17764f). Looks like your source archive is missing the MODULE.bazel file.

I saw that; the removal of the commented line is reverted, and since it’s diffing without a total file creation, there must be a MODULE.bazel in the tarball to diff against.

We don’t see the failure in the current run because presubmit is awaiting approval.

(Which is why I’ve asked if there’s a way to run the presubmit:

  1. would allow debugging locally,
  2. Faster code/test/debug cycle
  3. Currently doesn’t reproduce the error here or in 0.1.0

I don’t begrudge the need to check before permitting the test, but the delay in trying to debug the obscure error since rev 0.1.0, that’s getting frustrating — this release was an attempt to trim-back the things it was testing until it actually passes, then I could introduce the changes little by little until it breaks again as a way to bisect the failure to offer hints)

@fmeum
Copy link
Contributor

fmeum commented Dec 31, 2024

I don't see any non-validation failures on this PR. Do you have a link to the failed run? I can try to help figure out the difference between your local and the CI environment.

@chickenandpork
Copy link
Contributor

I don't see any non-validation failures on this PR. Do you have a link to the failed run? I can try to help figure out the difference between your local and the CI environment.

I think we'd be looking at https://buildkite.com/bazel/bcr-presubmit/builds/9716#01940d40-e586-4e9a-8d9e-9ffed8b685f5

I feels like the "ignore" logic in .bazelignore and/or the --deleted_packages login in .bazelrc is not working to ignore the //examples/ project/directory, or I can't use rules_bazel_integration_test as a dev_dependency = True and still load() any resource from there.

Noting again, of course, that the repro above (#issuecomment-2565971700) doesn't fail, so I'm getting signal that I can use resources from dev_dependency=True in a BUILD.bazel file.

@chickenandpork
Copy link
Contributor

Do you have a link to the failed run?

The investigation started back in https://buildkite.com/organizations/bazel/pipelines/bcr-bazel-compatibility-test/builds/72/jobs/019367bc-a6ce-4d3d-99b6-0b06aed7b344/log where the presubmit failed without indication. I don't know if this original problem is resolved because I could not repro the failure there either.

I think this was before I went down the rabbit-hole of @rules_bazel_integration_test replacing my bash-shell iteration of test examples. I was a. bit too impatient on this issue before trying to converge in more standard/reusable "hopefully best-practice" things.

@fmeum
Copy link
Contributor

fmeum commented Dec 31, 2024

The main test setup in the BCR uses an anonymous root module that has your tested module as a dependency. That's why you don't see dev dependencies. You can set up a dedicated test module that can bring in additional deps: https://github.com/bazelbuild/bazel-central-registry/blob/main/docs/README.md#test-module

@chickenandpork
Copy link
Contributor

The main test setup in the BCR uses an anonymous root module that has your tested module as a dependency. That's why you don't see dev dependencies

So this means we cannot use dev dependencies in any BUILD.bazel load()?

You can set up a dedicated test module
(re: failure in 0.0.2)
Yeah, that led to failures in 0.0.2: As a reason the @rules_synology// identifiers used as defaults in macros didn't exist as symbols, I guessed the bcr_test_module was importing as a different name. You can still see the history there in modules/rules_synology/0.0.2/presubmit.yml but I was running all tests (//...) to ensure everything was tested.

Because the failures weren't reproducible, I followed Alex Eagle's suggestion in Stack: (paraphrased): if I get something to work, I can patch in additional changes to the //modules/rules_synology/<version> content to see if it also works before merging -- so long as the presubmit-auto-run was active. That's currently I guess the only way to poke until something fails, but needs it to pass first hence the change away from bcr_test_module.

I've reverted the removed commented line (51d4fe3 reverts 69aec84) so maybe the "MODULE.bazel differs" check will pass us to the failure repro'd.

@chickenandpork
Copy link
Contributor

I've reverted the removed commented line (51d4fe3 reverts 69aec84) so maybe the "MODULE.bazel differs" check will pass us to the failure repro'd.

https://buildkite.com/organizations/bazel/pipelines/bcr-presubmit/builds/9801/jobs/01941e5e-30e4-4cb5-b0b3-61adb101bfb4/log (like 297) suggests I'm missing rules_proto

The only reference to rules_proto is in //examples/example-server:BUILD.bazel, but:

  1. examples/** is ignored in .bazelignore
  2. .bazelrc lists examples/example-server in build --deleted_packages=

(Basically BOTH means of ignoring test code as written up in the use of rules_bazel_integration_test)

This error doesn't repro on my side because the ignored directories are ignored, but if it's visible on use as a dependency, then we need another way of ignoring it.

Patch it out of existence?

@chickenandpork
Copy link
Contributor

after bd52bea we see failures:

  1. essentially, it seems that one test fails due to inability to download the bazel-8.0.0 binary
  2. the changes in bd52bea (appending -@rules_synology//examples/...) are ineffective to ignore that directory, so a dependency marked as dev_dependency=True is not present when the ignored //examples/BUILD.bazel is read

Overloading --deleted_packages in the build_targets array won't work, it's after a -- in the resulting command.

@fmeum
Copy link
Contributor

fmeum commented Jan 1, 2025

Does anything else reference targets under the examples package? I also don't see why this would happen.

@meteorcloudy Maybe you do?

@meteorcloudy
Copy link
Member

-@rules_synology//examples/... doesn't actually avoid loading the package.

.bazelrc lists examples/example-server in build --deleted_packages=

.bazelrc doesn't work transitively, so it has no affect when you build the project as a dependency.

.bazelignore does work transitively, but it doesn't exactly follow the .gitignore syntax.
The correct content should be just:

examples

and you may also need to exclude docs.

With .bazelignore being

# ignore the examples which are used to document suggested usage, validate this project, and -- in future -- repro issues
examples
docs

I was able to run bazel build --registry=file:///Users/pcloudy/workspace/bazel-central-registry -- @rules_synology//... successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants