issuedigger[bot]
commented
2 months ago The most similar issues to this one are:
-
76 , with a similarity score of 0.93.
-
106 , with a similarity score of 0.91.
-
141 , with a similarity score of 0.91.
Closed lukasjuhrich closed 1 month ago
issuedigger[bot]
commented
2 months ago The most similar issues to this one are:
lukasjuhrich
commented
2 months ago Sorry @issuedigger, these don't seem to be related :-)
alexpovel
commented
2 months ago Hi @lukasjuhrich , thanks for your report!
Apologies for @issuedigger, it's an experiment and safe to ignore.
To debug this further:
-vvvv, which is trace and therefore maximum level of verbosity, then post that output again here, please?pycroft a repo I can fetch?Initial thoughts:
let's confirm the command you invoke is doing what you hope it's doing. To reiterate:
it is searching for (Python) function calls within methods, and replacing every character x it finds with the string super. It does so recursively. For that, it will walk and look at all Python files (either with .py suffix or with a python3 or python shebang) starting at /home/lukas/code/10ag/pycroft.
I'm only mentioning this because the order of arguments you specify is unusual. It's legal and works, but for example I didn't see the x at first! It's easier to miss. The x does not apply to the --py methods part at all if that's what you're after.
/tmp/srgnXapArn etc. This step seems to fail. Is there some special setup involved on your machine regarding /tmp, beyond tmpfs?My FS setup/OS is different from yours, but I've run srgn on a vanilla Ubuntu 24 machine and it worked fine. I'll try that again if you can help me with reproduction steps!
alexpovel
commented
1 month ago Oh and to try and clear up any confusion more: you titled the issue "two scopes", but your command actually uses 3:
--py methods (only regard methods)--py function-calls (only regard function calls within methods aka the previous language scope)x (only regard the literal character x within all previous scopes); the fact that x was called before --py function-calls doesn't matter for this sort of orderingIt then also has one action:
super (replacement)If this is not what you intended, do you feel UX could be better? Documentation? Some fail-safe errors or warnings which could've helped you?
lukasjuhrich
commented
1 month ago Thank you very much for your insightful response! Indeed, the error is with replacing. As you conjectured, I did not intend to replace anything.
We can simplify the example by creating a test.py with contents print(min(3, 5)).
❯ srgn --py function-calls min
test.py
1:print(min(3, 5))
❯ srgn --py function-calls min max
test.py
[2024-10-08T14:27:02.918687Z ERROR srgn] Error walking at /home/lukas/code/10ag/pycroft/test/test.py due to: I/O error at path /tmp/srgnvBNwyP: Invalid cross-device link (os error 18)
[2024-10-08T14:27:02.918705Z ERROR srgn] Aborting walk for safety
Error: Error processing path: I/O error at path /tmp/srgnvBNwyP: Invalid cross-device link (os error 18)However, strace -f !! reveals some more information (truncated to the relevant part):
[pid 2307447] ioctl(2, TCGETS, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
[pid 2307447] write(2, "\33[90m[\33[0m2024-10-08T14:34:42.90"..., 118[2024-10-08T14:34:42.905065Z DEBUG srgn] Got new file contents, writing to file: "test.py"
) = 118
[pid 2307447] openat(AT_FDCWD, "/tmp/srgnmKcnCp", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 3
[pid 2307447] ioctl(2, TCGETS, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
[pid 2307447] write(2, "\33[90m[\33[0m2024-10-08T14:34:42.90"..., 113[2024-10-08T14:34:42.905232Z TRACE srgn] Writing to temporary file: "/tmp/srgnmKcnCp"
) = 113
[pid 2307447] write(3, "print(max(3, 5))\n", 17) = 17
[pid 2307447] renameat(AT_FDCWD, "/tmp/srgnmKcnCp", AT_FDCWD, "test.py") = -1 EXDEV (Invalid cross-device link)
[pid 2307447] unlink("/tmp/srgnmKcnCp") = 0
[pid 2307447] close(3) = 0
[pid 2307447] ioctl(2, TCGETS, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
[pid 2307447] write(2, "\33[90m[\33[0m2024-10-08T14:34:42.90"..., 215 <unfinished ...>
[pid 2307450] <... clock_nanosleep resumed>0x776875bff5d0) = 0
[pid 2307449] <... clock_nanosleep resumed>0x77686fdff5d0) = 0
[2024-10-08T14:34:42.905502Z ERROR srgn] Error walking at /home/lukas/code/10ag/pycroft/test/test.py due to: I/O error at path /tmp/srgnmKcnCp: Invalid cross-device link (os error 18)
[pid 2307450] clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=1000000}, <unfinished ...>
[pid 2307449] clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=1000000}, <unfinished ...>
[pid 2307447] <... write resumed>) = 215
[pid 2307448] <... clock_nanosleep resumed>0x776875fff5d0) = 0
[pid 2307447] ioctl(2, TCGETS <unfinished ...>
[pid 2307448] clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=1000000}, <unfinished ...>
[pid 2307447] <... ioctl resumed>, {c_iflag=ICRNL|IXON|IUTF8, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
[pid 2307447] write(2, "\33[90m[\33[0m2024-10-08T14:34:42.90"..., 97[2024-10-08T14:34:42.905694Z ERROR srgn] Aborting walk for safety
) = 97The renameat syscall error confirms that the rename is a problem, precisely because /tmp and <destination dir> lie on different file systems.
man 2 renameat confirms that this should not work:
ERRORS
[…]
EXDEV oldpath and newpath are not on the same mounted filesystem. (Linux permits a filesystem to be mounted at multiple points, but rename() does not work across different mount points, even if the same filesystem is mounted on both.)uname -a says Linux juhrilaptop 6.10.3-arch1-2 #1 SMP PREEMPT_DYNAMIC Tue, 06 Aug 2024 07:21:19 +0000 x86_64 GNU/Linux.
Are you sure you have a proper tmpfs in your environment, and not just a /tmp folder?
I'm not sure how to solve this, but let me present what I think the problems are.
First, that I can "push forward" the search argument (in this case, x) is a surprise to me, because the scope arguments are order dependent. This causes me to think of the whole chain of arguments as a pipeline, and even though I think I've read the docs telling me otherwise, I was convinced that I was searching for method bodies containing x and then narrowing further.
Second, I think it is very dangerous to have a destructive operation so "close" to a nondestructive one. I can think of two scenarios by which one can switch from search to replace by accident:
--py methods --py function-calls super and--py scopes to narrow it downsrgn … | less and accidentally removing or forgetting to type the pipe characterI tend to be afraid of tools which punish me for mistakes like that, because I tend to make many. I feel this is contrary to the "fast" and "light-weight" tool that srgn aims to be (and for me already is) if I have to be careful like that all the time; this is nontrivial cognitive load.
I can think of two options:
replace subcommand, ideally with an abbreviation like r, so I can use srgn r ….--go flag you can place wherever you like, where omission would cause a "dry run" of the replacement.Both require changing the default to "don't replace unless told to", which might require a deprecation period.
Re documentation, I think changing the semantics is more desirable than documenting the existing semantics better for the reasons I've outlined.
I guess this would bettor belong in a disussion instead of an issue, but since you asked, let me tell you of my use case: I wanted to find all the super() function calls inside methods without a certain decorator attached (@overload). I have not yet found out how to do so or whether srgn can do this.
igor47
commented
1 month ago i'm having the same issue with a simple rename operation (srgn --python 'imports' app.core.celery_app app.celery_app). like the OP, i have the repo i'm working in on a different device/filesystem from my /tmp .
igor47
commented
1 month ago i believe the issue is due to the way the dependency tempfile works. here:
https://github.com/alexpovel/srgn/blob/9238fc161ceaf88dd754d71b4fb65393dcee964b/src/main.rs#L541-L543
we declare file to be the result of tempfile::Builder (random question, i'm new to rust -- how the heck is tempfile even a valid symbol in this scope without a use tempfile at the top of the module?). this is probably created in the system temporary directory (/tmp on most linuxes). we then, after performing some replacement, call file.persist. the documentation for persist: https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#method.persist
Note: Temporary files cannot be persisted across filesystems. Also neither the file contents nor the containing directory are synchronized, so the update may not yet have reached the disk when persist returns.
probably because persist tries to create a hardlink to the temp file at the persisted destination, and hardlinks do not work across filesystems. hard to tell from all the code, definitely a hard-link is in one of the paths...
probably, we should be using the tempfile_in argument to Builder and creating the temp files in a local temporary directory created at the start of the run to avoid potential cross-filesystem hardlink issues.
alexpovel
commented
1 month ago Thanks @lukasjuhrich and @igor47 for your detailed reports. I'll look into it more soon.
The tempfile with persist was just me trying to be smart/safe, but it's optional. It would be pretty easy to just use a more standard approach here, which fixes the issue. I'm mainly confused how I hadn't caught this myself, as I tested with a very vanilla Ubuntu machine where I thought /tmp as tmpfs is the default. I'll double check that.
As for the UX, agree! At the very least, the two positional arguments of [SCOPE] [REPLACEMENT] should be forced to follow any and all options and flags. Then they stick out more. That could be a quick fix.
Re: how tempfile is in scope: because the module path is fully qualified. use is not strictly required, just reads better often.
Re:
I wanted to find all the super() function calls inside methods without a certain decorator attached (@overload)
Yeah don't think that's possible! If at all, it'd require substantial regex use. The without part is not really supported, srgn doesn't have a concept of negation.
alexpovel
commented
1 month ago Hey folks,
version 0.13.3 is released via #135 , which should fix this issue (the original one). #149 was created to track improvements to the UX, for which there's various options. More feedback, suggestions or even collaboration welcome!
As soon as a temporary file is involved, the temporary file seems to cause problems:
stdout / stderr
FS setup
/tmp tmpfs tmpfs rw,nosuid,nodev,nr_inodes=1048576,inode64/home /dev/mapper/vghdd-lvhome ext4 rw,relatime,data=orderedHow can I debug this further?