bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
249 stars 121 forks source link

Revert deleted options #327

Closed lakshmansai closed 1 year ago

lakshmansai commented 1 year ago

Fixes Issue https://github.com/bazeltools/bazel-deps/issues/325

Ran all unit test cases

 bazel-deps git:(revert_deleted_options) bazel test //...
INFO: Analyzed 102 targets (0 packages loaded, 0 targets configured).
INFO: Found 94 targets and 8 test targets...
INFO: Elapsed time: 28.071s, Critical Path: 27.96s
INFO: 12 processes: 1 internal, 5 darwin-sandbox, 6 worker.
INFO: Build completed successfully, 12 total actions
//test/scala/com/github/johnynek/bazel_deps:graphtest           (cached) PASSED in 2.6s
//test/scala/com/github/johnynek/bazel_deps:modeltest           (cached) PASSED in 1.2s
//test/scala/com/github/johnynek/bazel_deps:normalizertest      (cached) PASSED in 1.1s
//test/scala/com/github/johnynek/bazel_deps:coursier_test                PASSED in 21.8s
//test/scala/com/github/johnynek/bazel_deps:createpomtest                PASSED in 6.5s
//test/scala/com/github/johnynek/bazel_deps:parsegenerateddoctest        PASSED in 26.8s
//test/scala/com/github/johnynek/bazel_deps:parsetest                    PASSED in 1.0s
//test/scala/com/github/johnynek/bazel_deps:parsetestcases               PASSED in 1.0s

Executed 5 out of 8 tests: 8 tests pass.
INFO: Build completed successfully, 12 total actions

Tested generation of build files by running the command

 bazel-deps git:(revert_deleted_options) ✗ bazel run  //:parse -- generate -r /Users/Lakshman.Sai/Workspace/apollo/ -s thirdparty/generated-bazel-maven-deps.bzl -d dependencies.yaml             
INFO: Analyzed target //:parse (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //src/scala/com/github/johnynek/bazel_deps:parseproject up-to-date:
  bazel-bin/src/scala/com/github/johnynek/bazel_deps/parseproject
  bazel-bin/src/scala/com/github/johnynek/bazel_deps/parseproject.jar
INFO: Elapsed time: 0.106s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
wrote 291 targets

Compared the output by running the old version of bazel-deps and with this diff.

johnynek commented 1 year ago

Thanks for the PR. I'll review in detail next week.

Could you add a few comments to help me review?

Like I'd this only adding back previous code or has some been rewritten?

Can you add a comment as to how you think the previous default behavior can be restored for people who have moved to the current style? I think this flips the defaults back, doesn't it?

lakshmansai commented 1 year ago

I have added back the previous code didn't rewrite.

The defaults are not changed, depending on the options provided to generate command output files will be generated.

AFAIK options provided in README.md like -s to generate bazel files have been deleted in the PR https://github.com/bazeltools/bazel-deps/pull/321 So I don't think people have moved to the new style.

only the deleted options are added back so new options like resolved-output will still be working.

lionelfleury commented 1 year ago

Would be great to get that one forward. For now without -s option, the tool is broken for us.

johnynek commented 1 year ago

Thanks.

It's hard to review this because the PR is so large and many of the changes are just formatting changes.

This will break us, unfortunately, since the sha file arg -s is required here, but not even an option in the previous version.

I'll merge this and fix it up for us.

Thanks for taking the time.