Do not prepend the output path to an already-absolute output name

[Zaretto]

Fix issue when FGOutputFile::SetOutputName joins the configured output path with the supplied name, which produces a malformed path when the name is itself absolute.

i.e.; Take an absolute name as-is; only relative names are resolved under the output path.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.10%. Comparing base (0b99c70) to head (c083df8).

Files with missing lines	Patch %	Lines
src/input_output/FGOutputFile.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1475      +/-   ##
==========================================
- Coverage   25.10%   25.10%   -0.01%     
==========================================
  Files         171      171              
  Lines       18843    18844       +1     
==========================================
  Hits         4731     4731              
- Misses      14112    14113       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bcoconni]

I am in two minds with regard to this PR, do we really want to bypass the output path using absolute paths?

• @Zaretto could you please describe the use case where you need to specify an absolute path rather than setting the output path to the required location?
• @seanmcleod70 @agodemar any thoughts?

[Zaretto]

On Linux and similar operating systems you can already do this implicitly by setting name="/folder/file" however that doesn't work on Windows where I want to do name="c:/folder/file".

I had problems using scripts on Windows to create output where it needed to be rather than in the aircraft (or script folder) that prevented me from using the scripts in a single library for testing variants of models.

I guess this is more of a bugfix than a new feature.

[bcoconni]

I had problems using scripts on Windows to create output where it needed to be rather than in the aircraft (or script folder)

There is the method FGFDMexec::SetOutputPath that allows to avoid exactly that:

jsbsim/src/FGFDMExec.h

Lines 342 to 351 in 32aa2af

	/** Set the directory where the output files will be written.
	Relative paths are taken from the root directory.
	@param path path to the directory under which the output files will be
	written.
	@see SetRootDir
	@see GetOutputPath */
	bool SetOutputPath(const SGPath& path) {
	OutputPath = GetFullPath(path);
	return true;
	}

Are you using it ? It's a much more portable and versatile way to prevent your aircraft and script folders from being polluted by output files.

[seanmcleod70]

But there doesn't seem to be a command line argument, unless I'm missing it, to set the output path?

So it looks like JSBSim defaults to the current directory?

jsbsim/src/JSBSim.cpp

Line 397 in 32aa2af

FDMExec->SetOutputPath(SGPath("."));

And if you're writing a C++ wrapper or using the Python wrapper then you can call FGFDMExec::SetOutputPath(), but if you're running the standard JSBSim executable then you're stuck with the current directory?

jsbsim/src/JSBSim.cpp

Lines 842 to 863 in 32aa2af

	cout << " options:" << endl;
	cout << " --help returns this message" << endl;
	cout << " --version returns the version number" << endl;
	cout << " --outputlogfile=<filename> sets (overrides) the name of a data output file" << endl;
	cout << " --logdirectivefile=<filename> specifies the name of a data logging directives file" << endl;
	cout << " (can appear multiple times)" << endl;
	cout << " --root=<path> specifies the JSBSim root directory (where aircraft/, engine/, etc. reside)" << endl;
	cout << " --aircraft=<filename> specifies the name of the aircraft to be modeled" << endl;
	cout << " --script=<filename> specifies a script to run" << endl;
	cout << " --realtime specifies to run in actual real world time" << endl;
	cout << " --nice specifies to run at lower CPU usage" << endl;
	cout << " --nohighlight specifies that console output should be pure text only (no color)" << endl;
	cout << " --suspend specifies to suspend the simulation after initialization" << endl;
	cout << " --initfile=<filename> specifies an initialization file" << endl;
	cout << " --planet=<filename> specifies a planet definition file" << endl;
	cout << " --catalog specifies that all properties for this aircraft model should be printed" << endl;
	cout << " (catalog=aircraftname is an optional format)" << endl;
	cout << " --property=<name=value> e.g. --property=simulation/integrator/rate/rotational=1" << endl;
	cout << " --simulation-rate=<rate (double)> specifies the sim dT time or frequency" << endl;
	cout << " If rate specified is less than 1, it is interpreted as" << endl;
	cout << " a time step size, otherwise it is assumed to be a rate in Hertz." << endl;
	cout << " --end=<time (double)> specifies the sim end time" << endl << endl;

My understanding is that @Zaretto had script files/aircraft files with <output/> elements and wanted the ability to specify a full path in the <output/> element. Although I'm not sure how he's running things, i.e. just the regular JSBSim executable or not.

<output name="B737_datalog.csv" type="CSV" rate="20">
   <property> velocities/vc-kts </property>
   <velocities> ON </velocities>
</output>

@Zaretto I'm not sure why you don't think this is an issue in Linux since your example name="/folder/file" surely will only work once the concatenation is done resulting in ./folder/file if folder is a sub-directory of the current working directory or the current working directory is the root of the file system.

But if you want an absolute path from the root then you will also run into this issue on Linux.

[bcoconni]

but if you're running the standard JSBSim executable then you're stuck with the current directory?

True. But that will no longer be the case once the PR #1478 will be merged 😉

Note also that as you mentioned, the output files are by default written to the current directory when using the executable JSBSim.exe. And in that case, the output files would not be in the aircraft or scripts folder.

[seanmcleod70]

Hmm, in terms of 'current' directory, I had thought of mentioning above that @Zaretto could potentially change the current directory to where he wanted his output file to go, but I only just tried that for interest now with:

C:\source\temp\FlightTests>C:\source\jsbsim\Debug\JSBSim.exe --root=\source\jsbsim\ C:\source\jsbsim\scripts\737_cruise.xml

Where 737_cruise.xml has <output name="737_cruise_output.csv" type="CSV" rate="10"....

However I noticed that 737_cruise_output.csv didn't appear in C:\source\temp\FlightTests, instead it appeared in the root directory that I had specified.

I guess the .\737_cruise_output.csv is being re-directed relative to the root via this code, i.e. so it's not relative to the current directory?

jsbsim/src/FGFDMExec.h

Lines 715 to 720 in d89e357

	SGPath GetFullPath(const SGPath& name) {
	if (name.isRelative())
	return RootDir/name.utf8Str();
	else
	return name;
	}

[Zaretto]

Are you using it? It's a much more portable and versatile way to prevent your aircraft and script folders from being polluted by output files.

Yes, I am, and keeping the aircraft and script folders clean was exactly part of what I needed to solve. So we agree on the goal.

My problem was twofold.

First, setting the output to an absolute path on Windows (including the drive specification) produced mangled paths. An absolute name only works while the output path is unset. Once an output path is set, the name is appended to it:

• POSIX: /abs/file.csv is silently appended to the output path as <outdir>/abs/file.csv
• Windows: the result is i:/outdir/i:/abs/file.csv, a drive letter mid-path, which cannot be opened

That is why I see this as a bug fix rather than a feature change. Absolute output names are already accepted, they are just silently corrupted once an output path is also set.

Second, I genuinely needed the output in two different places on disk, and a single output path only gives me one. For example, the parameter log from the test script goes to one location and the aircraft debug files to another. At the time I could not build JSBSim from source and had to use a prebuilt executable, so I worked around it with a batch file that ran JSBSim and then shuffled the output into the right places afterwards. Honouring absolute names lets JSBSim write each output to its final location directly and removes that step.

[bcoconni]

OK. Thanks for the explanations @Zaretto.
Looks good to me.

[bcoconni]

I guess the .\737_cruise_output.csv is being re-directed relative to the root via this code, i.e. so it's not relative to the current directory?

jsbsim/src/FGFDMExec.h

Lines 715 to 720 in d89e357

	SGPath GetFullPath(const SGPath& name) {
	if (name.isRelative())
	return RootDir/name.utf8Str();
	else
	return name;
	}

Indeed. I had forgotten this subtlety.

[seanmcleod70]

I'd suggest a slight rewording of the comment:

// An absolute name is used as-is; only a relative name is taken under the
// output path.

// If fname is a relative filepath then prepend the configured output path, 
// otherwise use the absolute filepath.

[bcoconni]

I'd suggest a slight rewording of the comment:

// An absolute name is used as-is; only a relative name is taken under the
// output path.

// If fname is a relative filepath then prepend the configured output path, 
// otherwise use the absolute filepath.

Yep! And move it in the Doxygen comments/docs just above:

jsbsim/src/input_output/FGOutputFile.h

Lines 97 to 102 in c9d0f18

	/** Overwrites the name identifier under which the output will be logged.
	For this method to take effect, it must be called prior to
	FGFDMExec::RunIC(). If it is called after, it will not take effect before
	the next call to SetStartNewOutput().
	@param name new name */
	void SetOutputName(const std::string& fname) override {

[agodemar]

Great catch @Zaretto, I also had similar problems. The PR looks good to me too.

[seanmcleod70]

Looks good.