plantuml-stdlib / C4-PlantUML

C4-PlantUML combines the benefits of PlantUML and the C4 model for providing a simple way of describing and communicate software architectures
MIT License
6.38k stars 1.1k forks source link

-DRELATIVE_INCLUDE does not work (as expected) #240

Closed tfc closed 1 year ago

tfc commented 2 years ago

Hi,

i packaged a version of plantuml with the totally awesome C4 library in nixpkgs, so you can run nix-shell -p plantuml-c4 and then within that shell just run plantuml-c4 my-c4-diagram.uml without further setup, but also offline and reproducibly.

While doing so i experienced that setting -DRELATIVE_INCLUDE does not work at all: https://github.com/NixOS/nixpkgs/pull/192483/files#diff-9de1fe7c42caf79351ddb7ada270e1d0a82c30535f6836e507e1ea1f3c134b94R3

What works is:

Does this indicate that there is something wrong with my setup, or is the README simply outdated?

Potherca commented 2 years ago

I think the README might be wrong. If I remember correctly, system variables need to be passed before the JAR is called (otherwise they are passed as parameter to the jar file).

That means the correct form would be :

java -DRELATIVE_INCLUDE="." -jar plantuml.jar  ...
kirchsth commented 2 years ago

@Potherca: I think the readme is correct (e.g. it is used in the build pipeline with the percy/cli with the args: -v percy -o _parsed -DRELATIVE_INCLUDE="." ). I assume its a nix related problem, but I didn't analyze it)

tfc commented 2 years ago

i just tried it to confirm, but it does not work. It is very unlikely that this is a nix-related problem, as what i wrote there is just a bash wrapper that calls a normal JRE and a normal JAR. Trying the same list and order of arguments in the shell manually yields the same results.

@kirchsth the example you mentioned uses it with a local path like "." which i would assume to be a trivially working default. My suspicion is that it would also not work in the percy pipeline if it used a different path than the current working directory.

Potherca commented 2 years ago

Looks like the order is important (at least on Linux):

``` $ java -DPLANTUML_LIMIT_SIZE="1234" -jar plantuml.jar -version PlantUML version 1.2022.6 (Tue Jun 21 19:34:49 CEST 2022) (GPL source distribution) Java Runtime: OpenJDK Runtime Environment JVM: OpenJDK 64-Bit Server VM Default Encoding: UTF-8 Language: en Country: US PLANTUML_LIMIT_SIZE: 1234 Dot version: dot - graphviz version 2.43.0 (0) Installation seems OK. File generation OK ``` ``` $ java -jar plantuml.jar -version -DPLANTUML_LIMIT_SIZE="1234" PlantUML version 1.2022.6 (Tue Jun 21 19:34:49 CEST 2022) (GPL source distribution) Java Runtime: OpenJDK Runtime Environment JVM: OpenJDK 64-Bit Server VM Default Encoding: UTF-8 Language: en Country: US PLANTUML_LIMIT_SIZE: 4096 Dot version: dot - graphviz version 2.43.0 (0) Installation seems OK. File generation OK ````

I'll try to come up with a minimal working testcase for RELATIVE_INCLUDE

kirchsth commented 2 years ago

I checked the java help and PlantUML docu

java --help

    -D<name>=<value>
                  set a system property
...
   -version      print product version to the error stream and exit

plantuml

    -DVAR1=value        To set a preprocessing variable as if '!define VAR1 value' were used
...
    -version            To display information about PlantUML and Java versions

Maybe there are some dependencies that it has to be defined via a java setting

tfc commented 2 years ago

Looking at the code of the lib files:

' file: C4_Component.puml
!if %variable_exists("RELATIVE_INCLUDE")
  !include %get_variable_value("RELATIVE_INCLUDE")/C4_Container.puml
!else
  !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Container.puml
!endif

' ...

it looks like the variable RELATIVE_INCLUDE by itself alone was never meant to make includes work like this: !include C4_Container.puml. Otherwise they would be included like this, right?

If that is correct, then the readme should at least mention -Dplantuml.include.path="..."

tfc commented 2 years ago

I just realized that i might have mis-interpreted the readme in general.

To be independent of any internet connectivity, you can also download the files found in the root and activate the local conversion with additional command line argument -DRELATIVE_INCLUDE="." (that the local files are included)

This seems to mean that you're generally supposed to put the C4-files into the root and then use -DRELATIVE_INCLUDE="." with no other path than ".".

What i have built - including a completely different non-root-path - seems to be a not-yet supported scenario (nonetheless, it works great), may that be the case?

Potherca commented 2 years ago

The testcase I have created might clarify things a but further.

@startuml

rectangle "getenv("RELATIVE_INCLUDE"): %getenv("RELATIVE_INCLUDE")"

rectangle "get_variable_value("RELATIVE_INCLUDE"): %get_variable_value("RELATIVE_INCLUDE")"

@enduml
Call Result
java -DRELATIVE_INCLUDE="." -jar plantuml.jar test.puml image
java -jar plantuml.jar test.puml -DRELATIVE_INCLUDE="." image
kirchsth commented 2 years ago

HI @tfc,

This seems to mean that you're generally supposed to put the C4-files into the root and then use -DRELATIVE_INCLUDE="." with no other path than "."

Your assumption is correct (I used it atm only for the build pipeline) therefore I tried a fix with PR #252.

Can you please copy the files from my extended branch and check it?

Thank you and best regards Helmut

tfc commented 2 years ago

hi @kirchsth,

thank you for providing a patch. I tried it out by bumping to the latest commit of the branch you linked to and removing the C4 path from the additional -Dplantuml.include.path=... that the JAR had to be prefixed with (my understanding is that this should no longer be necessary). That unfortunately doesn't work, with the same error message as earlier. It does however also not break anything in the current package that i built around that.

kirchsth commented 2 years ago

hi @tfc,

did you test it only with the "nix calls"? Or did you tried it with the "java calls" from PR?

tfc commented 2 years ago

Hi @kirchsth,

the "nix calls" are simple java calls. What the nix package does is wrap the equivalent of the "java calls", which can be easily proven with strace.

Running the integration test of the package (with your latest patches) with the c4 folder in the -Dplantuml.include.path and -DRELATIVE_INCLUDE yields the correct working result:

(i shortened it down to the relevant parts and indented it for better readability)

$ nix-build . -A pkgs.plantuml-c4.passthru.tests.example-c4-diagram
# ...
execve(
  "/nix/store/nqghir73axma5bcj1nqc965dbyvx1hnx-openjdk-17.0.4+8/bin/java", 
  [ "/nix/store/nqghir73axma5bcj1nqc965dbyvx1hnx-openjdk-17.0.4+8/bin/java"
  , "-Dplantuml.include.path=/nix/store/s9768a2jcrkd9iih7jyc1j9fd8k18a6z-source:/nix/store/50q5vq58i73fnxzi9r1zdp2fxjcwrjrp-source"
  , "-jar", "/nix/store/wbs12lp1br3n26l7jjpga93ygbch4xl2-plantuml-1.2022.10/lib/plantuml.jar"
  , "-DRELATIVE_INCLUDE=/nix/store/s9768a2jcrkd9iih7jyc1j9fd8k18a6z-source"
  , "sample.puml"
  , "-o", "/nix/store/s2sgyza4g9qfj75s5dv7p43w756vc9m4-c4-plantuml-sample.png"
  ], 0x4ff010 /* 57 vars */) = 0
# ...
+++ exited with 0 +++
# ...
/nix/store/s2sgyza4g9qfj75s5dv7p43w756vc9m4-c4-plantuml-sample.png

Dropping the c4 folder from -Dplantuml.include.path and only keeping it in -DRELATIVE_INCLUDE:

$ nix-build . -A pkgs.plantuml-c4.passthru.tests.example-c4-diagram
# ...
execve(
  "/nix/store/nqghir73axma5bcj1nqc965dbyvx1hnx-openjdk-17.0.4+8/bin/java", 
  ["/nix/store/nqghir73axma5bcj1nqc965dbyvx1hnx-openjdk-17.0.4+8/bin/java"
  , "-Dplantuml.include.path=/nix/store/50q5vq58i73fnxzi9r1zdp2fxjcwrjrp-source"
  , "-jar", "/nix/store/sdnaqw63nqx5i5bvf0fxgy58xynpmy9i-plantuml-1.2022.10/lib/plantuml.jar"
  , "-DRELATIVE_INCLUDE=/nix/store/s9768a2jcrkd9iih7jyc1j9fd8k18a6z-source"
  , "sample.puml"
  , "-o", "/nix/store/la0akkp17vv2vkw202zgvnw0l5gdqzyk-c4-plantuml-sample.png"
  ], 0x4ff010 /* 57 vars */) = 0
# ...
Error line 2 in file: sample.puml
Some diagram description contains errors
+++ exited with 200 +++

What version of java do you use? The proprietary jvm, or the free one?

kirchsth commented 2 years ago

Hi @tfc, can you add your used sample.puml too? (I see "C4_Context Diagram Sample - enterprise.puml" in the nix source , but it contains only an url based include)

tfc commented 2 years ago

hi @kirchsth

You're right, it copies the C4_Context Diagram Sample - enterprise.puml from the samples folder of the C4 plantuml repo. It not only copies the example file but drops the https... prefix so that the filepath is without slashes and just the filename of the to-be-included file. In this case that is C4_Context.puml.

In my personal checkout of nixpkgs i only adjusted the commit hash here with the commit of your PR.

Edit: So for your convenience to reproduce this, this is the file's content after it ran through the sed filter:

@startuml "enterprise"
!include C4_Context.puml
' uncomment the following line and comment the first to use locally
' !include C4_Context.puml

LAYOUT_TOP_DOWN()
'LAYOUT_AS_SKETCH()
LAYOUT_WITH_LEGEND()

Person(customer, "Customer", "A customer of Widgets Limited.")

Enterprise_Boundary(c0, "Widgets Limited") {
    Person(csa, "Customer Service Agent", "Deals with customer enquiries.")

    System(ecommerce, "E-commerce System", "Allows customers to buy widgts online via the widgets.com website.")

    System(fulfilment, "Fulfilment System", "Responsible for processing and shipping of customer orders.")
}

System(taxamo, "Taxamo", "Calculates local tax (for EU B2B customers) and acts as a front-end for Braintree Payments.")

System(braintree, "Braintree Payments", "Processes credit card payments on behalf of Widgets Limited.")

System(post, "Jersey Post", "Calculates worldwide shipping costs for packages.")

Rel_R(customer, csa, "Asks questions to", "Telephone")

Rel_R(customer, ecommerce, "Places orders for widgets using")

Rel(csa, ecommerce, "Looks up order information using")

Rel_R(ecommerce, fulfilment, "Sends order information to")

Rel_D(fulfilment, post, "Gets shipping charges from")

Rel_D(ecommerce, taxamo, "Delegates credit card processing to")

Rel_L(taxamo, braintree, "Uses for credit card processing")

Lay_D(customer, braintree)

@enduml
kirchsth commented 2 years ago

Hi @tcf,

ok ... now I see the differences in your first statement (-Dplantuml.include.path != -DRELATIVE_INCLUDE; and you didn't duplicate it in the command line arguments)

1) You want to define an additional (absolute) include path via -Dplantuml.include.path (I found the related description search path) and -DRELATIVE_INCLUDE is only a switch that the included C4... does not load the other C4... files via URL.

I didn't use the -Dplantuml.include.path argument until now.

2) My solution was that "your nix" sample.puml starts like

@startuml "enterprise"
!if %variable_exists("RELATIVE_INCLUDE")
  !include %get_variable_value("RELATIVE_INCLUDE")/C4_Container.puml
!else
  !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Container.puml
!endif
...

and not with a simple !include C4_Context.puml, therefore it is not working for you without -Dplantuml.include.path

@Potherca: should I extend PR #252 with an additional -DLOAD_LOCAL=1 option

all C4_... files:

@startuml
!if %variable_exists("RELATIVE_INCLUDE") || %variable_exists("LOAD_LOCAL")
  !include ./C4_Container.puml
!else
  !include https://raw.githubusercontent.com/plantuml-stdlib/C4-PlantUML/master/C4_Container.puml
!endif

Sample.puml file

@startuml
!include C4_Container.puml

that a local version can be started with following arguments too

java -Dplantuml.include.path="c:/mydir" -jar plantuml.jar -DLOAD_LOCAL=1 ...
Potherca commented 2 years ago

@kirchsth I think at this point, it might be worth taking a step back and writing out what the scenarios are that we want to support and what the goal of each scenarios is. For me, at least, there are currently too many moving parts to feel confident with any suggestion I could make.

kirchsth commented 2 years ago

@Potherca ok I decouple the PR from the issue and merge the PR fix.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in the past 60 days. It will be closed in seven days if no further activity occurs. Thank you for your contributions.