CreatePom parsing update with maven type and classifier tags #358

jakestebbins · 2023-09-07T17:58:35Z

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 · 2023-09-07T19:04:18Z

src/scala/com/github/johnynek/bazel_deps/CreatePom.scala

@@ -7,7 +7,9 @@ object CreatePom {
    def toXml: Elem = {
      <dependency>
        <groupId>{self.group.asString}</groupId>
-        <artifactId>{self.artifact.asString}</artifactId>
+        <artifactId>{self.artifact.asString.split(":")(0)}</artifactId>


why is this split needed? why isn't it just self.artifact.artifactId?

Since we're concatenating the packaging and classifier in MavenArtifactId this pulls out just the ArtfiactId string. In the current state the MavenArtifactId.asString is really the artifact id, classifier, and packaging all concatenated together.

but look here:

bazel-deps/src/scala/com/github/johnynek/bazel_deps/DepsModel.scala

Line 459 in b5c7231

artifactId: String,

isn't self.artifact.asString.split(":")(0) == self.artifact.artifactId?

I understand the concatenation, but I'm saying access it directly before the concatenation. Note the confusing artifact vs artifactId difference. I'm not saying self.artifact.asString I'm saying self.artifact.artifactId.

Oh gotcha yeah that makes sense! will update that's much clearer

johnynek · 2023-09-07T19:04:53Z

thanks for the PR.

Just one question/suggestion.

johnynek

thanks for the PR

johnynek · 2023-09-07T20:14:55Z

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

jakestebbins · 2023-09-07T20:36:45Z

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 · 2023-09-07T20:48:53Z

can you merge master in and see if that fixes?

johnynek · 2023-09-07T20:52:23Z

I think we had the CI setup wrong: #359

johnynek · 2023-09-07T21:55:15Z

looks like we do have a test failure.

johnynek · 2023-09-07T23:38:43Z

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

jakestebbins · 2023-09-07T23:45:12Z

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 · 2023-09-08T01:41:32Z

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 · 2023-09-08T17:08:18Z

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 · 2023-09-13T19:55:04Z

@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 · 2023-09-20T00:57:00Z

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 · 2023-09-20T17:44:42Z

@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 · 2023-09-20T19:52:52Z

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

jakestebbins · 2023-09-20T20:14:16Z

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 · 2023-09-20T20:36:02Z

huh... failed again

jakestebbins · 2023-09-20T20:58:30Z

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

johnynek · 2023-09-20T21:19:14Z

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 · 2023-09-20T21:21:59Z

Sweet, thanks so much for all of the help!

johnynek · 2023-09-20T21:50:28Z

thank you! I appreciate your help!

initial commit with changes and tests

6f9ffe7

jakestebbins mentioned this pull request Sep 7, 2023

Creating Pom leaves ":" in artifactId leading to Maven issues #357

Open

johnynek reviewed Sep 7, 2023

View reviewed changes

jakestebbins added 2 commits September 7, 2023 12:38

fix typo in test

6ce90b0

switching to self.artifact.artifactId

d25c887

johnynek approved these changes Sep 7, 2023

View reviewed changes

Merge branch 'master' into jakestebbins/pom-parsing

cf4e9c2

fix typo in test

2175ba4

refactor None classifier and test update

815d041

jakestebbins added 2 commits September 13, 2023 13:00

fixing copy pasta string split

887e747

adding correct test case for new tags and refactored createPom

5a298db

jakestebbins requested a review from johnynek September 19, 2023 22:05

johnynek merged commit 815d28a into bazeltools:master Sep 20, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CreatePom parsing update with maven type and classifier tags #358

CreatePom parsing update with maven type and classifier tags #358

jakestebbins commented Sep 7, 2023

johnynek Sep 7, 2023

jakestebbins Sep 7, 2023

johnynek Sep 7, 2023

jakestebbins Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek left a comment

johnynek commented Sep 7, 2023

jakestebbins commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

jakestebbins commented Sep 7, 2023

johnynek commented Sep 8, 2023

jakestebbins commented Sep 8, 2023 •

edited

jakestebbins commented Sep 13, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

CreatePom parsing update with maven type and classifier tags #358

CreatePom parsing update with maven type and classifier tags #358

Conversation

jakestebbins commented Sep 7, 2023

johnynek Sep 7, 2023

Choose a reason for hiding this comment

jakestebbins Sep 7, 2023

Choose a reason for hiding this comment

johnynek Sep 7, 2023

Choose a reason for hiding this comment

jakestebbins Sep 7, 2023

Choose a reason for hiding this comment

johnynek commented Sep 7, 2023

johnynek left a comment

Choose a reason for hiding this comment

johnynek commented Sep 7, 2023

jakestebbins commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

johnynek commented Sep 7, 2023

jakestebbins commented Sep 7, 2023

johnynek commented Sep 8, 2023

jakestebbins commented Sep 8, 2023 • edited

jakestebbins commented Sep 13, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 20, 2023

johnynek commented Sep 20, 2023

jakestebbins commented Sep 8, 2023 •

edited