1031 fix java docker conflict #1077

mh182 · 2023-03-09T17:40:07Z

This is a major refactoring of dtcw (I would rather say it is a rewrite). So it may make more sense to look at the complete file instead of a diff.

The main goal of the refactoring was to make the code better understandable. Please provide feedback if this is true.

Note: the refactoring in dtcw is not meant to be complete.

I'm going to focus in the PR on the implementation details. I will explain the user visible changes in #1031.

What changed:

I replaced the different flags which controlled how dtcw behaves with a hash-map.
I started to split the code into functions in the hope to make the code better readable.
Improved test code
- The tests mock the downloads for fast execution (no internet needed)
- Split out the end-to-end tests into separate files (e2e_*.bats)
- Improved dtcw coverage
- Use git submodules to install the test framework. This way we can use a standard Debian container image for the tests instead of a home-brewed image.
I found and fixed various bugs (for example, incorrect handling of JAVA_HOME)

The open questions we should discuss (marked with # TODO in dtcw):

I don't fully understand how DTC_HEADLESS and -Dgradle.user.home work with Docker containers. When do we have to set which variable?
DTC_SITETHEME is never used (the environment flag to the docker container is never passed)
What is DTC_PROJECT_BRANCH good for? (not described anywhere but passed into dtcw)
Why do we check for .doctoolchain/jdk/Content/Home when looking for a JRE?
generateDeck can only be called in a local environment - is this correct?

What is missing:

Test coverage (see the test/README.adoc file)

netlify · 2023-03-09T17:40:13Z

✅ Deploy Preview for dtc-docs-preview ready!

Name	Link
🔨 Latest commit	`2605b5c`
🔍 Latest deploy log	https://app.netlify.com/sites/dtc-docs-preview/deploys/644ba486601588000737f01e
😎 Deploy Preview	https://deploy-preview-1077--dtc-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mh182 · 2023-03-10T07:18:08Z

The Header rules failed.
I looked into the GitHub workflows but could not find when and how those workflow is executed.
The deployment needs to be adapted.

mh182 · 2023-03-14T15:38:28Z

Since I got the dtcw tests now working with macOS, if found a major problem with my refactoring.

macOS uses, due to licensing reasons, an ancient bash version, more precise bash 3.2.

And bash arrays were introduced with bash 4. Since my refactoring uses associative arrays (hash map), it's not going to work on macOS.

mh182 · 2023-03-15T11:04:09Z

@rdmueller I have a question: does the current dtcw script work on a default bash provided by macOS, or do we expect people updating their bash version with Homebrew?

The bash version on macOS 12 is 3.5.57

$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin18)
Copyright (C) 2007 Free Software Foundation, Inc.

I could try to change dtcw that it works without index and associative arrays. The code is just going to be hard to maintain. And it doesn't seem that Apple is going to upgrade Bash on their system.

So my question is: should we make Bash >= 4 for dtcw a requirement with easier to maintain code or should we not?

mh182 · 2023-03-15T15:33:27Z

I found a solution to both problems:

GitHub Actions filters branch-ignoreand path-ignore (or path) don't work well together.
Regarding the problem with an old Bash version: I replaced index and associative arrays with strings. Seems to work.

One problem remains: the output and behavior of dtcw bash version is now quite different from the MS Windows version. Do we live with that or could you change dtcw.ps1 based on my changes?

mh182 · 2023-03-18T12:36:03Z

@rdmueller Could you please take a look at the changes?

The issues we have to address:

How can I fix the broken "Header rules" in dtcw-docs-preview step above?
Decision: do we provide this change as 2.3.0 - dtcw has changes in the CLI when installing Java/docToolchain/revealjs. I really would like to get this into production, since it contains bug fixes in corner cases with the different environments.
Does this version work when we deploy the Docker image (I'm not sure how gradle.user.home plays with this)?
There is a URL in dtcw's output (https://doctoolchain.github.io/docToolchain/#wsl) which leads to nowhere - what do we put in there?
In general, look at the code marked with # TODO maybe you can provide some insights

If we decide this goes into 2.3.0 we have to change the documentation on the page.

rdmueller · 2023-03-27T11:25:31Z

I don't fully understand how DTC_HEADLESS and -Dgradle.user.home work with Docker containers. When do we have to set which variable?

DTC_HEADLESS is used to supress asking if the external theme should be downloaded. This is quite important when on a CI system.

-Dgradle.user.home is the location where dependencies are cached. The docker image provides hopefully all dependencies and the parameter points gradle to those.

DTC_SITETHEME is never used (the environment flag to the docker container is never passed)

it is used withing generateSite. You can specify an external theme which hill be downloaded if it is locally not available. DTC_HEADLESS is used to avoid asking if it should be downloaded

What is DTC_PROJECT_BRANCH good for? (not described anywhere but passed into dtcw)

I think it is mainly used to link to the correct branch for the improve this doc link on the right. You can use the branch to create the base link in the config.

Why do we check for .doctoolchain/jdk/Content/Home when looking for a JRE?

As far as I remember, the structure of the zipped JDK is different for Windows than for Linux. The windows one contains the /Content folder.

generateDeck can only be called in a local environment - is this correct?

shouldn't be this way but it currently is. Within the docker image, reveal is already cloned into the image:

https://github.com/docToolchain/docker-image/blob/ng-beta/alpine/Dockerfile#L52

But the wrapper ignored it, because something it wrong with the following block:

https://github.com/docToolchain/docToolchain/blob/ng/dtcw#L122

it doesn't recognize reveal as already cloned. Workaround is not to use ´generateDeckas first parameter../dtcw docker install generateDeck will work. (dockeris removed as first parameter andinstall` is a nearly empty task)

rdmueller · 2023-03-27T11:27:03Z

The Header rules failed.

afaik, this is a check coming from netlify.

argh. netlify can't build the website because of the changed dtcw...

12:47:17 PM:   1. Build command from Netlify app                             
12:47:17 PM: ────────────────────────────────────────────────────────────────
12:47:17 PM: 
12:47:17 PM: $ ./netlifyBuildPreview.sh
12:47:17 PM: dtcw dev - ##DTCW_GIT_HASH##
12:47:17 PM: docToolchain 2.2.1
12:47:17 PM: Available docToolchain environments: local
12:47:17 PM: Environments with docToolchain [2.2.1]: none
12:47:17 PM: Using environment: local
12:47:17 PM: Error: doctoolchain - command not found [environment 'local']
12:47:17 PM: It seems docToolchain 2.2.1 is not installed. dtcw supports the
12:47:17 PM: following docToolchain environments:
12:47:17 PM: 1. 'local': to install docToolchain in [/opt/buildhome/.doctoolchain] use
12:47:17 PM:     $ ./dtcw local install doctoolchain
12:47:17 PM: 2. 'sdk': to install docToolchain with SDKMAN! (https://sdkman.io/)
12:47:17 PM:     # First install SDKMAN!
12:47:17 PM:     $ curl -s "https://get.sdkman.io/" | bash
12:47:17 PM:     # Then open a new shell and install docToolchain with
12:47:17 PM:     $ sdk install doctoolchain 2.2.1
12:47:17 PM: Note that running docToolchain in 'local' or 'sdk' environment needs a
12:47:17 PM: Java runtime (major version 8, 11, or 14) installed on your host.
12:47:17 PM: 3. 'docker': pull the docToolchain image and execute docToolchain in a container environment.
12:47:17 PM:     $ ./dtcw docker tasks --group doctoolchain

rdmueller · 2023-03-27T11:29:26Z

@rdmueller I have a question: does the current dtcw script work on a default bash provided by macOS, or do we expect people updating their bash version with Homebrew?

current version seems to work on macOS. Maybe we could try to detect outdated versions and ask the user to upgrade?

rdmueller · 2023-03-27T11:31:24Z

One problem remains: the output and behavior of dtcw bash version is now quite different from the MS Windows version. Do we live with that or could you change dtcw.ps1 based on my changes?

I think we should align both. I can take a look at it.

rdmueller

thanx for the re-write. Seems to me that you are a bash wizard!

I commented on two things I would like to change.
And for the rest I have to admit that I am not too good at bash scripting to fully understand everything. So I would like to give it a try by releasing this as some kind of release candidate.

What do you think?

And btw - we should increase the version to 1.0, shouldn't we?

dtcw

mh182 · 2023-03-27T17:55:50Z

One problem remains: the output and behavior of dtcw bash version is now quite different from the MS Windows version. Do we live with that or could you change dtcw.ps1 based on my changes?

I think we should align both. I can take a look at it.

This would be fine. I would not merge this change into ng without the change in dtcw.ps1.

We really should think about what technology we should use for v3. Having 3 scripts for the same logic is error prone.

mh182 · 2023-03-27T17:57:11Z

The Header rules failed.

afaik, this is a check coming from netlify.

argh. netlify can't build the website because of the changed dtcw...

Can we change this per branch basis?

I never worked with netlify, so I have no idea where to adapt this.

mh182 · 2023-03-27T18:05:27Z

I don't fully understand how DTC_HEADLESS and -Dgradle.user.home work with Docker containers. When do we have to set which variable?

DTC_HEADLESS is used to supress asking if the external theme should be downloaded. This is quite important when on a CI system.

I think this one is set correctly.

-Dgradle.user.home is the location where dependencies are cached. The docker image provides hopefully all dependencies and the parameter points gradle to those.

What is the default value if gradle.user.home is not provided? Or better, how could I test this?

Why do we check for .doctoolchain/jdk/Content/Home when looking for a JRE?

As far as I remember, the structure of the zipped JDK is different for Windows than for Linux. The windows one contains the /Content folder.

OK, I will add this information in the code.

generateDeck can only be called in a local environment - is this correct?

shouldn't be this way but it currently is. Within the docker image, reveal is already cloned into the image:

https://github.com/docToolchain/docker-image/blob/ng-beta/alpine/Dockerfile#L52

But the wrapper ignored it, because something it wrong with the following block:

https://github.com/docToolchain/docToolchain/blob/ng/dtcw#L122

it doesn't recognize reveal as already cloned. Workaround is not to use ´generateDeckas first parameter../dtcw docker install generateDeck will work. (dockeris removed as first parameter andinstall` is a nearly empty task)

OK, so the generateDeck task should also be available in the other environments. I will have to take a look at this.

mh182 · 2023-03-27T18:10:29Z

thanx for the re-write. Seems to me that you are a bash wizard!

I commented on two things I would like to change. And for the rest I have to admit that I am not too good at bash scripting to fully understand everything. So I would like to give it a try by releasing this as some kind of release candidate.

What do you think?

And btw - we should increase the version to 1.0, shouldn't we?

I would bump the version to 0.50 and go for test period.

But first we have to adjust the changes in dtcw.ps1.

mh182 · 2023-03-29T13:13:19Z

I adapted the documentation for the installation instructions.

What we are missing now is the changes in the Windows wrapper scripts.

rdmueller · 2023-03-30T14:16:47Z

I currently try to get netlify preview up and running, but I get the following error:

4:08:32 PM: + ./dtcw local install doctoolchain
4:08:32 PM: dtcw dev - ##DTCW_GIT_HASH##
4:08:32 PM: docToolchain 2.2.1
4:08:32 PM: Available docToolchain environments: local
4:08:32 PM: Environments with docToolchain [2.2.1]: none
4:08:32 PM: Using environment: local
4:08:32 PM: Installed docToolchain successfully in '/opt/buildhome/.doctoolchain/docToolchain-2.2.1'.
4:08:32 PM: Error: unsupported Java version 8 [/usr/bin/java]
4:08:32 PM: docToolchain supports Java versions 8, 11 (preferred) or 14. In case one of those

any idea?

mh182 · 2023-03-30T14:28:21Z

I currently try to get netlify preview up and running, but I get the following error:

4:08:32 PM: + ./dtcw local install doctoolchain
4:08:32 PM: dtcw dev - ##DTCW_GIT_HASH##
4:08:32 PM: docToolchain 2.2.1
4:08:32 PM: Available docToolchain environments: local
4:08:32 PM: Environments with docToolchain [2.2.1]: none
4:08:32 PM: Using environment: local
4:08:32 PM: Installed docToolchain successfully in '/opt/buildhome/.doctoolchain/docToolchain-2.2.1'.
4:08:32 PM: Error: unsupported Java version 8 [/usr/bin/java]
4:08:32 PM: docToolchain supports Java versions 8, 11 (preferred) or 14. In case one of those

any idea?

I will add a test case for Java 8. Could you provide the output of /usr/bin/java -version please? Maybe I introduced an error in parsing the output.

I will add a new test case with the output.

mh182 · 2023-04-04T08:07:01Z

what is the meaning of

    [ -z "${installations}" ] && installations=" none"

${installations} is a string containing all environments in which docToolchain is installed.

The line above means if docToolchain is not installed (string empty), show none. This could be written with

if  [ -z "${installations}" ]; then
 Installations=" none"
fi

I use boolean logic as oneliner. The part after && is only executed if the first part is true.

rdmueller · 2023-04-05T21:02:06Z

making progress. nearly through

dtcw

rdmueller · 2023-04-06T21:01:26Z

so, the powershell version has been rewritten to match the rewritten bash version.
First manual tests are looking good. Still have to add more automated tests.

What are our next steps?

mh182 · 2023-04-07T05:58:16Z

What are our next steps?

Check and fix the documentation, address the minor issues from your feedback, manual testing, then integrate it.

Regarding the documentation: have you found the reason why the formatting breaks at the end of the installation section?

I will do that next week after my vacation.

rdmueller · 2023-04-07T09:55:02Z

Regarding the documentation: have you found the reason why the formatting breaks at the end of the installation section?

I just checked it and couldn't find it anymore?!?

mh182 · 2023-04-07T10:23:39Z

Then we have just to update the documentation (output) from the windows ps1 (and bat) scripts.

Since you have a Mac, where do you test the ms windows environment?

rdmueller · 2023-04-07T11:53:42Z

Since you have a Mac, where do you test the ms windows environment?

Since a few weeks, I own a Mac with M2pro chip for work and I do have a windows machine as my private workhorse. This gives me the flexibility to test everything.

PacoVK

Great rewrite! I skimmed through the change and added some comments that may ease the rebase on ng

test/install_reveal_js.bats

test/java_environment.bats

dtcw

dtcw.ps1

test/e2e_local_install.bats

test/install_reveal_js.bats

mh182 · 2023-04-25T09:46:36Z

I rebased this PR and removed the code to install reveal.js.

mh182 · 2023-04-25T16:55:54Z

I really would like to get this PR into the ng to avoid continuous rebasing.

@rdmueller Is the re-write of dtcw.ps1 complete?

rdmueller · 2023-04-25T17:02:13Z

@mh182 , yes, the rewrite should be complete. If we.merge it now, I also have time to fix upcoming problems quickly

mh182

Minor issues to fix Last fixes before we can integrate.

dtcw

src/docs/010_manual/20_install.adoc

changelog.adoc

src/docs/010_manual/20_install.adoc

rdmueller

I am quite happy with these changes. let's merge!

dtcw

- Fix bug docToolchain#1031 - Fix: support of JAVA_HOME which was silently ignored. - Add `--version` option - dtcw API change: use explicit 'install' command to install components locally. - Improve output: provide the concept of 'environments'. - Show deprecation of 'getJava' - Skip 'install doctoolchain' if already installed Refactorings: - Replace boolean flags which `available_environments` and `dtc_installations`. Tests: - Use mocks for dtcw test suite to improve performance. - Added end-to-end tests for installation in 'local' and 'sdk' environments.

mh182 linked an issue Mar 9, 2023 that may be closed by this pull request

dtcw ignores installed Java RE when docker is installed - your java version 17 is too new #1031

Closed

This was referenced Mar 9, 2023

Integrate dtcw tests into CI build #1073

Closed

dtcw effectively not running in non-interactive mode #867

Closed

mh182 force-pushed the 1031_fix_java_docker_conflict branch 2 times, most recently from 2f493dd to 36e0d8c Compare March 14, 2023 14:49

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 36e0d8c to f00f05b Compare March 15, 2023 14:57

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 1b62119 to 9cc29fb Compare March 18, 2023 12:21

mh182 requested a review from rdmueller March 18, 2023 12:24

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 9cc29fb to 9293f53 Compare March 20, 2023 11:46

rdmueller requested changes Mar 27, 2023

View reviewed changes

dtcw Outdated Show resolved Hide resolved

dtcw Outdated Show resolved Hide resolved

mh182 requested a review from rdmueller March 27, 2023 18:10

mh182 force-pushed the 1031_fix_java_docker_conflict branch from e1cf745 to 57a3c36 Compare March 29, 2023 13:06

mh182 marked this pull request as ready for review March 29, 2023 13:11

rdmueller reviewed Apr 6, 2023

View reviewed changes

dtcw Outdated Show resolved Hide resolved

rdmueller reviewed Apr 6, 2023

View reviewed changes

dtcw Outdated Show resolved Hide resolved

PacoVK mentioned this pull request Apr 7, 2023

Upgrade Asciidoc Gradle Plugin #1098

Merged

3 tasks

This was linked to issues Apr 7, 2023

dtcw - sdkman installation check returns wrong result #621

Closed

getJava doesn't work without "local" #973

Closed

mh182 mentioned this pull request Apr 13, 2023

Upgrade Asciidoctor Gradle Plugin v4 #1105

Closed

PacoVK reviewed Apr 21, 2023

View reviewed changes

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 9ef4125 to d526c12 Compare April 25, 2023 09:45

rdmueller approved these changes Apr 25, 2023

View reviewed changes

mh182 force-pushed the 1031_fix_java_docker_conflict branch from d526c12 to 35ec1c2 Compare April 25, 2023 16:48

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 35ec1c2 to 1b08ce2 Compare April 26, 2023 10:10

mh182 commented Apr 26, 2023

View reviewed changes

dtcw Outdated Show resolved Hide resolved

src/docs/010_manual/20_install.adoc Show resolved Hide resolved

changelog.adoc Outdated Show resolved Hide resolved

src/docs/010_manual/20_install.adoc Show resolved Hide resolved

mh182 mentioned this pull request Apr 26, 2023

dtcw not backward compatible with docToolchain 2.* with Java 17: Unsupported class file major version 61 #1117

Closed

mh182 requested a review from rdmueller April 27, 2023 06:27

rdmueller approved these changes Apr 28, 2023

View reviewed changes

mh182 commented Apr 28, 2023

View reviewed changes

dtcw Outdated Show resolved Hide resolved

mh182 force-pushed the 1031_fix_java_docker_conflict branch from 08e91cc to 2605b5c Compare April 28, 2023 10:48

mh182 merged commit bd8b0e6 into docToolchain:ng Apr 28, 2023
11 checks passed

mh182 deleted the 1031_fix_java_docker_conflict branch May 4, 2023 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1031 fix java docker conflict #1077

1031 fix java docker conflict #1077

mh182 commented Mar 9, 2023

netlify bot commented Mar 9, 2023 •

edited

mh182 commented Mar 10, 2023

mh182 commented Mar 14, 2023

mh182 commented Mar 15, 2023

mh182 commented Mar 15, 2023

mh182 commented Mar 18, 2023

rdmueller commented Mar 27, 2023

rdmueller commented Mar 27, 2023 •

edited

rdmueller commented Mar 27, 2023

rdmueller commented Mar 27, 2023

rdmueller left a comment

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 29, 2023

rdmueller commented Mar 30, 2023

mh182 commented Mar 30, 2023

mh182 commented Apr 4, 2023 •

edited

rdmueller commented Apr 5, 2023

rdmueller commented Apr 6, 2023

mh182 commented Apr 7, 2023

rdmueller commented Apr 7, 2023

mh182 commented Apr 7, 2023

rdmueller commented Apr 7, 2023

PacoVK left a comment

mh182 commented Apr 25, 2023

mh182 commented Apr 25, 2023

rdmueller commented Apr 25, 2023

mh182 left a comment

rdmueller left a comment

1031 fix java docker conflict #1077

1031 fix java docker conflict #1077

Conversation

mh182 commented Mar 9, 2023

netlify bot commented Mar 9, 2023 • edited

✅ Deploy Preview for dtc-docs-preview ready!

mh182 commented Mar 10, 2023

mh182 commented Mar 14, 2023

mh182 commented Mar 15, 2023

mh182 commented Mar 15, 2023

mh182 commented Mar 18, 2023

rdmueller commented Mar 27, 2023

rdmueller commented Mar 27, 2023 • edited

rdmueller commented Mar 27, 2023

rdmueller commented Mar 27, 2023

rdmueller left a comment

Choose a reason for hiding this comment

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 27, 2023

mh182 commented Mar 29, 2023

rdmueller commented Mar 30, 2023

mh182 commented Mar 30, 2023

mh182 commented Apr 4, 2023 • edited

rdmueller commented Apr 5, 2023

rdmueller commented Apr 6, 2023

mh182 commented Apr 7, 2023

rdmueller commented Apr 7, 2023

mh182 commented Apr 7, 2023

rdmueller commented Apr 7, 2023

PacoVK left a comment

Choose a reason for hiding this comment

mh182 commented Apr 25, 2023

mh182 commented Apr 25, 2023

rdmueller commented Apr 25, 2023

mh182 left a comment

Choose a reason for hiding this comment

rdmueller left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 9, 2023 •

edited

rdmueller commented Mar 27, 2023 •

edited

mh182 commented Apr 4, 2023 •

edited