-
Notifications
You must be signed in to change notification settings - Fork 32
fix relative paths for METS file #1353
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested separately with my ocrd network configuration when reviewing. However, if the workspace paths resolve correctly with your network configuration, that should be good enough to be merged. After the new core release, I will still have to verify with the Operandi setup afterwards.
EDIT: To fix the single failing error, it should suffice to just remove the first 6 characters from the test string in this line.
| file_dict['local_filename'] = srcpath.name # will be copied-in below | ||
|
|
||
| destpath = Path(workspace.directory, file_dict['local_filename']) | ||
| if srcpath != destpath and not destpath.exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the ambiguity has already been resolved, could we use an underscore for src_path and dest_path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff will become bigger, but sure... (I'll wait for @kba first, though)
| args=["ocrd", "workspace", "-U", f"{mets_server_url}", "-d", f"{ws_dir_path}", "server", "start"], | ||
| stdout=open(file=log_file, mode="w"), stderr=open(file=log_file, mode="a"), cwd=ws_dir_path, | ||
| stdout=open(file=log_file, mode="w"), stderr=open(file=log_file, mode="a"), | ||
| #cwd=ws_dir_path, # rs: if relative, this will cause wrong path resolution in the subprocess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Potentially setting the current working directory to workspace directory path was the only way to make it work without changing/fixing the Workspace class itself or the workspace CLI. However, that had other side effects leading to a different bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes #1213