Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mktemp in tesstrain_utils.sh #2051

Merged
merged 2 commits into from Nov 15, 2018

Conversation

@johnlinp
Copy link
Contributor

commented Nov 14, 2018

The commit 10f2c45 unified the usage of mktemp, but with a
incorrect bash syntax and unnecessary definition of LANG_CODE
and TIMESTAMP. This patch fixes the above problems.

Fix mktemp in tesstrain_utils.sh
The commit 10f2c45 unified the usage of mktemp, but with a
incorrect bash syntax and unnecessary definition of LANG_CODE
and TIMESTAMP. This patch fixes the above problems.
@zdenop

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

I was looking for some good source for checking bash script quality and I found several references https://www.shellcheck.net/ which can be used online and offline.
Some of suggested changes it recognized as wrong (see inline comments), but it found several other issues. So I suggest to fix all 3 bash script with shellcheck.

@stweil

stweil approved these changes Nov 14, 2018

Copy link
Contributor

left a comment

The new code looks good – now the macro MKTEMP_DT delivers a new directory name each time when it is called. More simplifications are possible: mktemp -d -t can be used for all platforms.

@amitdo

This comment has been minimized.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

@stweil You are right - MKTEMP_DT can be simplified to mktemp -d -t only. I pushed a new commit for that change. Please take a look at it. Thank you.

@zdenop

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@johnlinp : I reply here and not in #2054, because here is right place for comments of this PR.

  1. This commit fix nothing: this is right statement, because there is no real problem that your commit solve fix. Changing syntax fix nothing.
  2. You claim you fix wrong bash syntax. This is true only for TMP_DIR=$(${MKTEMP_DT} ${LANG_CODE}-${TIMESTAMP}.XXX). I asked you for source (standard) that claim used syntax is wrong. I still did not get answer. Even I give you reference that new modern style for bash use $(...) only. And we try to modernize tesseract code.
  3. If you want to fix syntax you need to fix it in all file, to be consistent. Now script use syntaxt for variables double-quotes, back-quoted and $(...). Such changes are not improvement, because decrease code readability.

Comment report that this PR makes script even worse.

@zdenop

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

I am correcting myself: TMP_DIR=$(${MKTEMP_DT} ${LANG_CODE}-${TIMESTAMP}.XXX) and FONT_CONFIG_CACHE=$(${MKTEMP_DT} font_tmp.XXXXXXXXXX) seems to fix issue #2054.

But rest of my objection are IMO still valid.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

Hi @zdenop

  1. This commit fix nothing: this is right statement, because there is no real problem that your commit solve fix. Changing syntax fix nothing.

There is a real problem to solve: #2054.

  1. You claim you fix wrong bash syntax. This is true only for TMP_DIR=$(${MKTEMP_DT} ${LANG_CODE}-${TIMESTAMP}.XXX). I asked you for source (standard) that claim used syntax is wrong. I still did not get answer. Even I give you reference that new modern style for bash use $(...) only. And we try to modernize tesseract code.

In your commit 10f2c45, you wrote:

MKTEMP_DT=$(mktemp -d -t)

which directly execute the command mktemp -d -t. However, mktemp -d -t is not supposed to executed directly, it should be executed with a template argument. Executing mktemp -d -t will cause warning messages like mktemp: option requires an argument -- t, which is the message described in #2054.

  1. If you want to fix syntax you need to fix it in all file, to be consistent. Now script use syntaxt for variables double-quotes, back-quoted and $(...). Such changes are not improvement, because decrease code readability.

Comment report that this PR makes script even worse.

This commit doesn't mean to fix syntax. This commit is to fix the problem like #2054.

@stweil

stweil approved these changes Nov 15, 2018

Copy link
Contributor

left a comment

Now we could even remove the macro MKTEMP_DT. That would simplify the code further.

@stweil stweil merged commit edb76e2 into tesseract-ocr:master Nov 15, 2018

4 of 5 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stweil

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Merged, thank you. I added a commit to simplify the shell code even more.

@johnlinp johnlinp deleted the johnlinp:fix-tesstrain-utils-mktemp branch Nov 15, 2018

@zdenop

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

I am sorry I fooled myself that you are just changing syntax. My bad - I should test it :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.