bazeltools / bazel-deps

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

CreatePom parsing update with maven type and classifier tags #358

Closed jakestebbins closed 1 year ago

jakestebbins commented 1 year ago

I left the MavenArtifactId class untouched to just fix this in CreatePom and avoid breaking other things that depend on that class. I wasn't able to get bazel to run tests for me(even on master), but confirmed that mvn was able to handle the classifier and type tags using the same format as what's in the updated CreatePomTest!

johnynek commented 1 year ago

thanks for the PR.

Just one question/suggestion.

johnynek commented 1 year ago

I'm trying to figure out why I'm not being shown an option to approve your PR to run in the CI.

jakestebbins commented 1 year ago

Oh weird, I hope I forked/setup the PR the right way. Do you maybe have to be assigned instead of just a reviewer?

johnynek commented 1 year ago

can you merge master in and see if that fixes?

johnynek commented 1 year ago

I think we had the CI setup wrong: https://github.com/bazeltools/bazel-deps/pull/359

johnynek commented 1 year ago

looks like we do have a test failure.

johnynek commented 1 year ago

could you run bazel test //test/scala/com/github/johnynek/bazel_deps:createpomtest locally? looks like that is still failing.

jakestebbins commented 1 year ago

Yeah sorry I'm a bazel newbie so idk what my issue is but I keep getting errors in protobuf that causes it to fail building?

bazel test //test/scala/com/github/johnynek/bazel_deps:createpomtest
WARNING: Running Bazel server needs to be killed, because the startup options are different.
Starting local Bazel server and connecting to it...
INFO: Analyzed target //test/scala/com/github/johnynek/bazel_deps:createpomtest (156 packages loaded, 1670 targets configured).
INFO: Found 1 test target...
ERROR: /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/external/com_google_protobuf/BUILD:206:11: Compiling src/google/protobuf/any.pb.cc failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 42 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/com_google_protobuf/src/google/protobuf/any.pb.cc:30:5: error: member initializer 'type_url_' does not name a non-static data member or base class
  : type_url_(&::_pbi::fixed_address_empty_string, ::_pbi::ConstantInitialized{})
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
johnynek commented 1 year ago

looks like your version of bazel is incompatible with the current rules. Try with the script in the repo:

./bazel test ... note the ./ which points to a script in the root of the repo.

jakestebbins commented 1 year ago

Yeah I tried running the script, and also confirmed I'm running bazel v4.2.0, and I also tried bazelisk with no luck.

% ./bazel build //...                                                   
INFO: Analyzed 109 targets (4 packages loaded, 39 targets configured).
INFO: Found 109 targets...
INFO: From Compiling zutil.c:
external/zlib/zutil.c:133:22: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
const char * ZEXPORT zError(err)
                     ^
external/zlib/zutil.c:305:22: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
voidpf ZLIB_INTERNAL zcalloc (opaque, items, size)
                     ^
external/zlib/zutil.c:315:20: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
void ZLIB_INTERNAL zcfree (opaque, ptr)
                   ^
3 warnings generated.
INFO: From Compiling inftrees.c:
external/zlib/inftrees.c:32:19: warning: a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
int ZLIB_INTERNAL inflate_table(type, lens, codes, table, bits, work)
                  ^
1 warning generated.
ERROR: /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/external/com_google_protobuf/BUILD:155:11: Compiling src/google/protobuf/arenastring.cc failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 34 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/com_google_protobuf/src/google/protobuf/arenastring.cc:190:20: error: no member named 'IsAllocated' in 'google::protobuf::internal::TaggedStringPtr'; did you mean 'GetIfAllocated'?
  if (!tagged_ptr_.IsAllocated()) {
                   ^~~~~~~~~~~
                   GetIfAllocated
/usr/local/include/google/protobuf/arenastring.h:170:23: note: 'GetIfAllocated' declared here
  inline std::string *GetIfAllocated() const {
                      ^
external/com_google_protobuf/src/google/protobuf/arenastring.cc:219:19: error: no member named 'IsAllocated' in 'google::protobuf::internal::TaggedStringPtr'; did you mean 'GetIfAllocated'?
  if (tagged_ptr_.IsAllocated()) {
                  ^~~~~~~~~~~
                  GetIfAllocated
/usr/local/include/google/protobuf/arenastring.h:170:23: note: 'GetIfAllocated' declared here
  inline std::string *GetIfAllocated() const {
                      ^
2 errors generated.
ERROR: /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/BUILD:10:12 Building external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/scalac.jar (4 source files) failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 34 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
INFO: Elapsed time: 3.519s, Critical Path: 3.03s
INFO: 21 processes: 17 internal, 4 darwin-sandbox.
FAILED: Build did NOT complete successfully

 % bazel --version
bazel 4.2.0

% ./bazel test ... 
INFO: Analyzed 109 targets (0 packages loaded, 0 targets configured).
INFO: Found 101 targets and 8 test targets...
ERROR: /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/external/com_google_protobuf/BUILD:416:11: Compiling src/google/protobuf/compiler/plugin.pb.cc failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 42 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/com_google_protobuf/src/google/protobuf/compiler/plugin.pb.cc:27:5: error: member initializer 'suffix_' does not name a non-static data member or base class
  : suffix_(&::_pbi::fixed_address_empty_string, ::_pbi::ConstantInitialized{})
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/com_google_protobuf/src/google/protobuf/compiler/plugin.pb.cc:28:5: error: member initializer 'major_' does not name a non-static data member or base class
  , major_(0)
    ^~~~~~~~~
external/com_google_protobuf/src/google/protobuf/compiler/plugin.pb.cc:29:5: error: member initializer 'minor_' does not name a non-static data member or base class
  , minor_(0)
    ^~~~~~~~~
external/com_google_protobuf/src/google/protobuf/compiler/plugin.pb.cc:30:5: error: member initializer 'patch_' does not name a non-static data member or base class
  , patch_(0){}
    ^~~~~~~~~
external/com_google_protobuf/src/google/protobuf/compiler/plugin.pb.cc:25:29: warning: constexpr constructor that does not initialize all members is a C++20 extension [-Wc++20-extensions]
PROTOBUF_CONSTEXPR Version::Version(
                            ^
/usr/local/include/google/protobuf/compiler/plugin.pb.h:310:3: note: member not initialized by constructor
  union { Impl_ _impl_; };
  ^
jakestebbins commented 1 year ago

@johnynek sorry for the lag, finally got tests to pass I just need to add a test case for a dependency with a non-empty < classifier > tag

johnynek commented 1 year ago

thanks for sticking with it.

It seems the tests still aren't passing. I'm sorry the project doesn't build for you. It shouldn't depend on your environment, but I guess something in one of the rules we depend on does... Maybe we need to update the version of the scala rules. I really don't know.

jakestebbins commented 1 year ago

@johnynek weird the tests are passing for me locally, and just did a pull to make sure I'm up to date

`jakestebbins@Jake-Stebbins-MacBook-Pro bazel-deps % ./bazel test --cache_test_results=no //... INFO: Build option --cache_test_results has changed, discarding analysis cache. INFO: Analyzed 109 targets (0 packages loaded, 1709 targets configured). INFO: Found 101 targets and 8 test targets... INFO: Elapsed time: 40.015s, Critical Path: 39.49s INFO: 9 processes: 1 internal, 8 darwin-sandbox. INFO: Build completed successfully, 9 total actions //test/scala/com/github/johnynek/bazel_deps:coursier_test PASSED in 9.0s //test/scala/com/github/johnynek/bazel_deps:createpomtest PASSED in 5.3s //test/scala/com/github/johnynek/bazel_deps:graphtest PASSED in 2.3s //test/scala/com/github/johnynek/bazel_deps:modeltest PASSED in 2.6s //test/scala/com/github/johnynek/bazel_deps:normalizertest PASSED in 3.3s //test/scala/com/github/johnynek/bazel_deps:parsegenerateddoctest PASSED in 39.2s //test/scala/com/github/johnynek/bazel_deps:parsetest PASSED in 2.9s //test/scala/com/github/johnynek/bazel_deps:parsetestcases PASSED in 3.0s

Executed 8 out of 8 tests: 8 tests pass. INFO: Build completed successfully, 9 total actions jakestebbins@Jake-Stebbins-MacBook-Pro bazel-deps % `

johnynek commented 1 year ago

can you try --runs_per_test=10 and see if that passes.

jakestebbins commented 1 year ago

Yeah it looks like it just failed the parsegenerateddoctest once, but in the latest CI run its showing the tests passed and errored in the command run after

`jakestebbins@Jake-Stebbins-MacBook-Pro bazel-deps % ./bazel test --cache_test_results=no --runs_per_test=10 //... INFO: Build option --runs_per_test has changed, discarding analysis cache. INFO: Analyzed 109 targets (0 packages loaded, 1709 targets configured). INFO: Found 101 targets and 8 test targets... FAIL: //test/scala/com/github/johnynek/bazel_deps:parsegenerateddoctest (run 3 of 10) (see /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/execroot/com_github_johnynek_bazel_deps/bazel-out/darwin-fastbuild/testlogs/test/scala/com/github/johnynek/bazel_deps/parsegenerateddoctest/run_3_of_10/test.log) INFO: Elapsed time: 258.888s, Critical Path: 248.90s INFO: 81 processes: 1 internal, 80 darwin-sandbox. INFO: Build completed, 1 test FAILED, 81 total actions //test/scala/com/github/johnynek/bazel_deps:coursier_test PASSED in 35.8s Stats over 10 runs: max = 35.8s, min = 16.2s, avg = 29.0s, dev = 5.4s //test/scala/com/github/johnynek/bazel_deps:createpomtest PASSED in 25.5s Stats over 10 runs: max = 25.5s, min = 18.2s, avg = 21.5s, dev = 2.2s //test/scala/com/github/johnynek/bazel_deps:graphtest PASSED in 5.2s Stats over 10 runs: max = 5.2s, min = 3.9s, avg = 4.7s, dev = 0.4s //test/scala/com/github/johnynek/bazel_deps:modeltest PASSED in 8.6s Stats over 10 runs: max = 8.6s, min = 5.4s, avg = 6.7s, dev = 1.1s //test/scala/com/github/johnynek/bazel_deps:normalizertest PASSED in 11.0s Stats over 10 runs: max = 11.0s, min = 8.4s, avg = 10.0s, dev = 0.7s //test/scala/com/github/johnynek/bazel_deps:parsetest PASSED in 12.5s Stats over 10 runs: max = 12.5s, min = 8.6s, avg = 10.8s, dev = 1.5s //test/scala/com/github/johnynek/bazel_deps:parsetestcases PASSED in 19.8s Stats over 10 runs: max = 19.8s, min = 17.3s, avg = 18.6s, dev = 1.0s //test/scala/com/github/johnynek/bazel_deps:parsegenerateddoctest FAILED in 1 out of 10 in 248.3s Stats over 10 runs: max = 248.3s, min = 184.2s, avg = 221.0s, dev = 17.9s /private/var/tmp/_bazel_jakestebbins/bfa95731683341b674a6956238363bd9/execroot/com_github_johnynek_bazel_deps/bazel-out/darwin-fastbuild/testlogs/test/scala/com/github/johnynek/bazel_deps/parsegenerateddoctest/run_3_of_10/test.log

Executed 8 out of 8 tests: 7 tests pass and 1 fails locally. INFO: Build completed, 1 test FAILED, 81 total actions jakestebbins@Jake-Stebbins-MacBook-Pro bazel-deps % `

johnynek commented 1 year ago

huh... failed again

jakestebbins commented 1 year ago

Yeah I just ran it again with the same --runs_per_test=10 flag and it passed

johnynek commented 1 year ago

I don't see how your change could have broken this test since your change is downstream of the code being tested.

I'll merge and we can address the test.

Thanks!

jakestebbins commented 1 year ago

Sweet, thanks so much for all of the help!

johnynek commented 1 year ago

thank you! I appreciate your help!