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

Update genReport.sh #1222

Merged
merged 8 commits into from
Nov 7, 2023
Merged

Update genReport.sh #1222

merged 8 commits into from
Nov 7, 2023

Conversation

slhotellier
Copy link
Contributor

Add BIRTCLASSPATH

Add BIRTCLASSPATH
Copy link
Contributor

@hvbtup hvbtup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the script needs a general overhaul.

@slhotellier: Of course that isn't your mistake - the script was broken already before your changes. But you can help here! I'm running BIRT on Windows only.

First of all, I'm not sure if we actually need to export the variables. For example, BIRT_HOME is given to Java as a system property. I don't know for sure if BIRT uses the environment variable BIRT_HOME internally, but I doubt it, because Java good practice is to use system properties instead.

And then thera are some differences in comparison to the Windows script genReport.bat:

  • The environment variable javaXioXtmpdir is written differently. With dots in the Windows version, but underscores in the Linux version. Both versions are incorrect in a way: The correct way is to set the system property java.io.tmpdir, but neither the Windows nor the Linux script does this.
  • The base directory for this variable is set differently: Based on the script location in the Windows version, but based on the current working directory in the Linux version. I think this is a matter of taste in the end; I'd keep it as is.
  • The Windows script does not set the system property org.eclipse.datatools_workspacepath, but the Linux script does. Probably the Linux script is correct in this regard.
  • Most Important points are differences in handling of BIRTCLASSPATH and BIRT_HOME:

Using the trailing * is correct in both scripts (Windows and your patched Linux).
But the Windows script sets BIRTCLASSPATH relative to its own location, which is the correct way.
The Linux script (unpatched as well as patched) set this wrong. Note that $PWD can be different from the the script location.

The Windows script will set the set system property BIRT_HOME if and only if there is a platform subdirectory parallel to the lib directory, which is the correct way AFAIK (I only tested the Non-OSGI runtime on Windows).
For the Non-OSGI release, the system property BIRT_HOME must not be set.
The existence of the BIRT_HOME system property decides if OSGI mode is entered or not.
The Linux script (again, patched and unpatched) is incorrect in this regard.

And of course a fixed path like /opt/birt/birt-runtime is not allowed in this script.

@wimjongman wimjongman added this to the 4.14 milestone Mar 4, 2023
@wimjongman
Copy link
Contributor

@slhotellier are you going to work on this?

update BIRT_HOME to "" because the value must be specify by user (depending on install directory)
remove variable export (not use outside this shell)
@slhotellier
Copy link
Contributor Author

sorry for the delay.
I took the comments into account.
The exports were not necessary, I deleted them.

correction java version and typo error
@claesrosell
Copy link
Contributor

@slhotellier, you are free to squash/merge this PR. Thanks for your contribution!

@slhotellier slhotellier closed this Nov 2, 2023
@slhotellier slhotellier reopened this Nov 2, 2023
@claesrosell claesrosell merged commit 3089f99 into eclipse-birt:master Nov 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants