-
Notifications
You must be signed in to change notification settings - Fork 53
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
Use Grok instead of OpenJpeg #2
Conversation
Oops forgot instructions.
|
And it works with @dannylamb's terrible Tokyo JP2 from Islandora/documentation#572 (comment) |
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.
Just a few little nits the with the playbook, but it mostly looks good.
inventory/vagrant/group_vars/all.yml
Outdated
@@ -10,3 +10,7 @@ islandora_extra_packages: | |||
- unzip | |||
- build-essential | |||
- vim | |||
- cmake |
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 these packages are specific to the grok role. I would install them inside the grok role.
Maybe inside install.yml using a structure like this:
https://github.com/Islandora-Devops/claw-playbook/blob/66ef19d323a3c9cc1e1d3e88438d99d3abb0d6e1/roles/internal/openjpeg/tasks/install.yml#L3-L8
@@ -0,0 +1 @@ | |||
/usr/local/lib |
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.
Is this actually needed? I believe /usr/local/lib
is in the library path by default.
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 seemed to need it to decode JP2s, but Tiffs worked without it. Perhaps I just need the to run ldconfig
?
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.
For sure will need ldconfig
to load the libraries.
I think it should be included by default, I guess its not a big deal to define it twice.
|
||
- name: Add Grok libraries to ld.so.conf.d | ||
copy: src={{ item }} dest=/{{ item }} owner=root group=root mode=0444 | ||
with_items: |
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.
See comment about this not being necessary above. If we do include it, I wouldn't use a loop to install one item.
I'd rather see something like:
- name: Add Grok libraries to ld.so.conf.d
copy:
src: libgrok.conf
dest: /etc/ld.so.conf.d/libgrok.conf
owner: root
group: root
mode: "0444"
or alternatively in the single line syntax
- name: Add Grok libraries to ld.so.conf.d
copy: src=libgrok.conf dest=/etc/ld.so.conf.d/libgrok.conf owner=root group=root mode=0444
I personally prefer the YAML syntax to the single line, but thats just a style thing.
roles/internal/grok/vars/main.yml
Outdated
@@ -0,0 +1,4 @@ | |||
grok_clone_directory: /opt/grok | |||
grok_version_tag: v2.3.0 |
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 would put both of these in the roles/internal/grok/defaults/main.yml
file, as they are super hard to override where they are. Here is a good reference talking about where to put variables. The TL;DR is that you should always use the defaults, not vars.
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.
Right, I did have these there and then I stupidly moved the entire file to cause the below Cantaloupe option to get set. I'll fix them both.
roles/internal/grok/vars/main.yml
Outdated
grok_clone_directory: /opt/grok | ||
grok_version_tag: v2.3.0 | ||
|
||
cantaloupe_OpenJpegProcessor_path_to_binaries: /usr/local/bin |
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.
This shouldn't be defined in this playbook for modularity. You want this role to stand on its own not to be setting variables from other roles. I would set this in this with the rest of the cantaloupe variables in:
https://github.com/Islandora-Devops/claw-playbook/blob/master/inventory/vagrant/group_vars/tomcat.yml#L37-L53
make: | ||
chdir: "{{ grok_clone_directory }}/build" | ||
|
||
- name: Install grok |
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.
Did you run the role twice to make sure this is idempotent? Taking a quick glance at the cmake library that is included, I don't think that this will be idempotent without the creates statement, but I could be reading the code wrong and I haven't actually run it yet.
You can tell if its idempotent by running it twice and seeing if ansible reports any tasks as changed.
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.
So it does seem to have 'changed' it. But I also realized reading the code that he has a creates option, so I'll try adding that.
One other minor thing I forgot to mention is that I try to avoid python libraries as much as possible, simply because of the extra cognitive overhead of having to deal with python as well as all the ansible yaml, unless the library buys something thats hard to do in YAML. The big on is if a library provides idempotency, its probably worth it. In this case I'm not sure it buys us idempotency and I'm not sure its much more convenient then just doing something like: - name: Cmake grok
command:
cmake blah blah
args:
creates: file blah blah Just something else to consider. |
@jonathangreen ok I think I got it all. I do need to run |
Forgot some unused/unneeded variables and files. |
chdir: "{{ grok_clone_directory }}/build" | ||
creates: /usr/local/bin/opj_decompress | ||
|
||
- name: Update ldconfig |
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.
So to get rid of the last idempotency thing, I'd do something like this:
- name: Install grok
command: make install
args:
chdir: "{{ grok_clone_directory }}/build"
creates: /usr/local/bin/opj_decompress
register: grok_make_install
- name: Update ldconfig
command: ldconfig
when: grok_make_install.changed is defined AND grok_make_install.changed
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.
Your when:
line above caused an error when I tried to run it. I was able (I think) to get the expected process with when: grok_make_install.changed
. Does that seem viable?
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.
Looks good man. Couple other small things, but after that it should be good to go.
file: | ||
path: "{{ grok_clone_directory }}/build" | ||
state: directory | ||
mode: 0755 |
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.
weird ansible gotcha here, but always put the mode in quotes since its an octal number or it'll be interpreted as decimal, and shit will get weird haha.
ansible/ansible#9196
https://blog.tyk.nu/blog/ansible-and-unix-file-permissions/
Partially Resolves? Islandora/documentation#703
This removes the OpenJpeg library and instead downloads and compiles the Grok fork for use with Cantaloupe.