MarcusBarnes / mik

The Move to Islandora Kit is an extensible PHP command-line tool for converting source content and metadata into packages suitable for importing into Islandora (or other digital repository and preservations systems).
GNU General Public License v3.0
34 stars 11 forks source link

CSVBooks: MIK fails when output directory doesn't exist #457

Closed bondjimbond closed 5 years ago

bondjimbond commented 6 years ago

In CSV Single File, if you set an output_directory that doesn't exist yet, MIK creates it for you without problems.

In CSV Books, however, MIK fails. Logs the following error:

ErrorException.ERROR: ErrorException {"message":"mkdir(): No such file or directory"

If you create the output_directory and leave the config file alone, it runs just fine.

mjordan commented 6 years ago

I'll take care of this.

mjordan commented 6 years ago

@bondjimbond I just opened #459 and, strangely, did not see this behaviour while running my new code numerous times. If you have time to test that PR, can you confirm that the problem still exists?

bondjimbond commented 6 years ago

@MarcusBarnes Re the question of permissions: It shouldn't be a permissions thing, since it's only the CSV Books toolchain that fails to create the directory. Other toolchains (CSV Single File) do it without problems.

bondjimbond commented 6 years ago

Update: Error also occurs in CsvCompound toolchain.

mjordan commented 6 years ago

@bondjimbond I'm afraid I can't replicate the error for either toolchain. The only thing I can think of is that it has to do with being on a Mac. @MarcusBarnes are you able to replicate at all?

MarcusBarnes commented 6 years ago

@mjordan I'll schedule some time to attempt to reproduce the issue early this week and report back on this issue thread.

bondjimbond commented 5 years ago

@MarcusBarnes Any update on reproducing the problem?

MarcusBarnes commented 5 years ago

Confirming that for CSVBook, if you set an output_directory that doesn't exist yet, you get an error:

[2018-09-17 17:57:25] ErrorException.ERROR: ErrorException {"message":"mkdir(): No such file or directory","code" .. "file":"../src/writers/CsvBooks.php","line":96} []'

MarcusBarnes commented 5 years ago

Looking at CSVSingleFile, it uses the createOutputDirectory() method:

https://github.com/MarcusBarnes/mik/blob/master/src/writers/CsvSingleFile.php#L64

Following a similar pattern from CSVSingleFile in CSVBook from about L84 to L97 should help resolve this issue.

mjordan commented 5 years ago

I'm not saying my previous testing was iron-clad, but it's strange the I couldn't replicate this problem on Linux, and I use the compound toolchain frequently (not sow much the book toolchain) at work on Windows. @MarcusBarnes just to be sure, could you run the same config, etc. you used to confirm this on your Mac on a linux VM like the Islandora Vagrant? I'll also replicate my attempts to reproduce this on Linux.

I can't explain why in my attempts an output directory gets created despite the absence of the createOutputDirectory() method, which is even more reason for me to retry my attempts.

MarcusBarnes commented 5 years ago

@mjordan Despite my pull-request, I now know what the issue is now based on your most recent comment. In the createOutputDirectory method, we are explicitly providing the MODE parameter for mkdir, whereas in the CSVBook writer, we are not. This may introduce difference across systems.

Compare

https://github.com/MarcusBarnes/mik/blob/master/src/writers/Writer.php#L135

with

https://github.com/MarcusBarnes/mik/blob/master/src/writers/CsvBooks.php#L96

We can modify my pull-request or just leave it as is, since it's usuful for future contributions to CSVBook to know about the createOutputDirectory method from the parent Writer class.

See http://php.net/manual/en/function.mkdir.php for the PHP Documentation on the mkdir function.

mjordan commented 5 years ago

@MarcusBarnes good catch. Modifying your current PR is probably the most complete fix if you're OK with that.

mjordan commented 5 years ago

Just curious, does what you've discovered explain any differences between OSX and Linux? Windows probably doesn't use MODE at all...

MarcusBarnes commented 5 years ago

@mjordan I'm OK with modifying my pull-request. Do you want me to leave the call to createOutputDirectory?

mjordan commented 5 years ago

Yes, I think for consistency, we should be using that method everywhere, since it is defined in the parent class.

MarcusBarnes commented 5 years ago

Addressed in pull-request https://github.com/MarcusBarnes/mik/pull/489 (merged with commit https://github.com/MarcusBarnes/mik/commit/38514834edb30b4a2513e0638605ea23560bb3e2).