Skip to content
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

Fix reloading file with axis #2762

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sundtek
Copy link
Contributor

@sundtek sundtek commented Nov 27, 2023

This fixes following error:

Exception in Tkinter callback
Traceback (most recent call last):
File "/usr/lib/python3.7/tkinter/init.py", line 1705, in call return self.func(*args)
File "/home/sundtek/devel/linuxcnc/bin/axis", line 2796, in touch_off_system reload_file(False)
File "/home/sundtek/devel/linuxcnc/bin/axis", line 1911, in reload_file shutil.copyfile(loaded_file, tempfile)
File "/usr/lib/python3.7/shutil.py", line 104, in copyfile raise SameFileError("{!r} and {!r} are the same file".format(src, dst)) shutil.SameFileError: '/tmp/tmp3d2pd26j/sidewall.nc' and '/tmp/tmp3d2pd26j/sidewall.nc' are the same file

opening the tmp file will overwrite loaded_file, the patch backups the original loaded_file and sets loaded_file back to the original name after loading the temp file.

This fixes following error:

Exception in Tkinter callback
Traceback (most recent call last):
File "/usr/lib/python3.7/tkinter/init.py", line 1705, in call
return self.func(*args)
File "/home/sundtek/devel/linuxcnc/bin/axis", line 2796, in touch_off_system
reload_file(False)
File "/home/sundtek/devel/linuxcnc/bin/axis", line 1911, in reload_file
shutil.copyfile(loaded_file, tempfile)
File "/usr/lib/python3.7/shutil.py", line 104, in copyfile
raise SameFileError("{!r} and {!r} are the same file".format(src, dst))
shutil.SameFileError: '/tmp/tmp3d2pd26j/sidewall.nc' and '/tmp/tmp3d2pd26j/sidewall.nc' are the same file

opening the tmp file will overwrite loaded_file, the patch backups the original loaded_file and sets loaded_file back to the original name after loading the temp file.
sundtek referenced this pull request Nov 27, 2023
The issue here is that if you open a gcode file, and then change the
file on disk, you'll think your changes are completely inert because
they don't show up in the GUI, but actually LinuxCNC has its own
separate file handle behind-the-scenes, and your machine can do scary
and unpredictable things.

With this fix, the code being executed by LinuxCNC always matches what
is displayed in the AXIS GUI even if you subsequently change the file.
@andypugh
Copy link
Collaborator

Have you tested this with a filter file configured?
https://linuxcnc.org/docs/stable/html/config/ini-config.html#sub:ini:sec:filter

@sundtek
Copy link
Contributor Author

sundtek commented Nov 28, 2023

My patch will basically disable #2418 it's not a solution either, just a temporary workaround (don't pull).
I did not test the filters yet
Is the original author @jes still around (and would like to update it)?
By the way thanks for documenting your original pull request, even though it doesn't work as expected
it at least documents what you would like to achieve.

@rmu75 mentioned that linuxcnc is reloading the file when starting a job.

So "user" open should create a copy of the original file.
The reload function should have an option - reloaded by user or reloaded by the system.
Reload by user should replace the the copy / cached file.

@andypugh , @rmu75 what do you think about that?

@jes
Copy link
Contributor

jes commented Nov 28, 2023

I am still around but evidently don't understand enough about what is going on to recognise the problem, it works fine on my system.

I also don't see how your patch disables #2418 - it looks like it keeps it intact to me?

@sundtek
Copy link
Contributor Author

sundtek commented Nov 28, 2023

@rmu75 mentioned that there's a reload when starting the job, the file could be changed during that point. I will have a closer look at it during this week...
@jes Can you attach your working axis file?

@jes
Copy link
Contributor

jes commented Nov 28, 2023

I've attached the axis file from my system (as .txt, because github).

If I recall correctly, I made this by manually applying my patch to the version of axis that was already installed.

axis.txt

@rmu75
Copy link
Contributor

rmu75 commented Nov 28, 2023

I think I explained everything in #2490. Maybe I'm missing something, but behaviour here is constistent with that explanation.

First, a temporary filename is constructed from random but fixed temp dir location plus base filename of "loaded_file".
Second, in open_file_guts, "loaded_file" is overwritten with the temporary file name.

So when you reload, loaded_file already points to the temporary file and will try to copy it over itself (and miss any changes to the original file btw).

@jes
Copy link
Contributor

jes commented Nov 28, 2023

Oh, I hadn't realised there was a specific reload button. I always reload by reopening the same file, that'll be why I didn't notice anything wrong.

@andypugh
Copy link
Collaborator

Is this still a live PR? It sounds like it might have been overtaken by events?

@andypugh
Copy link
Collaborator

andypugh commented Aug 3, 2024

I am still confused about whether this bug still exists. (I do think it's a bug, you should need to reload the file to run the modified version, it should be safe to edit the file on disk.)
Though I guess this is a modern "infinite memory" point of view. Historically on systems with limited resources you would probably have expected the current file on disk to be executed from disk rather than from memory.

@jes
Copy link
Contributor

jes commented Aug 3, 2024

If you're asking for my view: I'm still just using the version from #2762 (comment) , which has my fix from #2418 (which breaks the reload button, which I never use).

But sadly I'm planning to move away from LinuxCNC soon anyway so my view doesn't count for much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants