Remove changing mode of files by ext_job

[ManInFez]

Issue
Resolves #1907

Pre review checklist

• Added appropriate labels

Adding labels helps the maintainers when writing release notes, see sections and the
corresponding labels here: https://github.com/equinor/ert/blob/main/.github/release.yml

[codecov-commenter]

Codecov Report

Merging #2784 (88590d1) into main (e5fa49c) will decrease coverage by 0.03%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main    #2784      +/-   ##
==========================================
- Coverage   65.21%   65.18%   -0.04%     
==========================================
  Files         653      653              
  Lines       53496    53492       -4     
  Branches     4790     4802      +12     
==========================================
- Hits        34887    34868      -19     
- Misses      17006    17012       +6     
- Partials     1603     1612       +9     

Impacted Files	Coverage Δ
libres/lib/job_queue/ext_job.cpp	62.46% <20.00%> (-1.77%)	⬇️
libres/lib/job_queue/ext_joblist.cpp	61.90% <0.00%> (-3.18%)	⬇️
libres/lib/res_util/block_fs.cpp	53.38% <0.00%> (-0.59%)	⬇️
libres/lib/analysis/update.cpp	40.51% <0.00%> (+0.25%)	⬆️
libres/lib/enkf/gen_kw.cpp	59.51% <0.00%> (+0.48%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5fa49c...88590d1. Read the comment docs.

[oyvindeide]

Looks like a good change 👍 Because this might cause some workflows and forward models to fail because they rely on this behavior, how does this fail if we dont have execute permission, is the error message understandable?

[ManInFez]

Looks like a good change +1 Because this might cause some workflows and forward models to fail because they rely on this behavior, how does this fail if we dont have execute permission, is the error message understandable?

It outputs an error message saying you do not have execution rights to this file

ert/libres/lib/job_queue/ext_job.cpp

Line 444 in ac2ea6e

fprintf(stderr,

[oyvindeide]

Looks like a good change +1 Because this might cause some workflows and forward models to fail because they rely on this behavior, how does this fail if we dont have execute permission, is the error message understandable?

It outputs an error message saying you do not have execution rights to this file

ert/libres/lib/job_queue/ext_job.cpp

Line 444 in ac2ea6e

fprintf(stderr,

It does not mention that the file could have been found, but did not have the correct rights though, maybe add that as well? Or check if the file is found, check the rights, and if not found, throw the very appropriate error (or print as it appears we do, though and error is probably better)?

[oyvindeide]

The reason I am asking so much is I once ended up trying to debug when this happened, and was not the first thing I thought of, so took me some time to figure out.

[ManInFez]

test libres please!

[ManInFez]

test ert please!

[oyvindeide]

Looks good, just running some checks, would it make sense to add a test to this?

[ManInFez]

test ert please!

[oyvindeide]

There is now a clear warning in the console if this occurs so a great improvement 👍 It is only a warning though, and cant quite agree with myself if I think it should be an error. If you try to actually use the forward model, you will get an error, but then it is not so clear, you have to put it in context with the previous warnings, for example you get:

Error message: ext_joblist_get_job_copy: asked for job:MY_CUSTOM_JOB which does not exist

See file: /tmp/ert_abort_dump.log for more details of the crash.
Setting the environment variable "ERT_SHOW_BACKTRACE" will show the backtrace on stderr.

then the warning is higher in the console:

Error parsing executable: ** You do not have execute rights to: /some/path/MY_CUSTOM_JOB** Warning: job: 'MY_CUSTOM_JOB' not available ...

maybe it would be better to error right away?

[ManInFez]

There is now a clear warning in the console if this occurs so a great improvement +1 It is only a warning though, and cant quite agree with myself if I think it should be an error. If you try to actually use the forward model, you will get an error, but then it is not so clear, you have to put it in context with the previous warnings, for example you get:

Error message: ext_joblist_get_job_copy: asked for job:MY_CUSTOM_JOB which does not exist

See file: /tmp/ert_abort_dump.log for more details of the crash.
Setting the environment variable "ERT_SHOW_BACKTRACE" will show the backtrace on stderr.

then the warning is higher in the console:

Error parsing executable: ** You do not have execute rights to: /some/path/MY_CUSTOM_JOB** Warning: job: 'MY_CUSTOM_JOB' not available ...

maybe it would be better to error right away?

This will cause the python constructor to segfault when something is wrong, as it will not be able to capture the exception. I think the scope of the issue becomes a little too large for this PR.

[oyvindeide]

This will cause the python constructor to segfault when something is wrong, as it will not be able to capture the exception. I think the scope of the issue becomes a little too large for this PR.

I know, it is not ideal, but it ends up doing that anyway when it ulimately calls `util_abort, and the message just becomes a bit less clear because it says it did not find the job. Could for catch the exceptions and accumulate them before reraising with the full message. And either not catch it at all, or recatch even higher up.

[ManInFez]

I would prefer to not change this behaviour in this PR, but I can make an issue for it?