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

Define pre-proc vars for FORMTMP and FORMTMPSORT. #433

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

Conversation

jodavies
Copy link
Collaborator

@jodavies jodavies commented Mar 6, 2023

Sometimes these paths can be useful in a form script, for use with #system, for example.

Sometimes these paths can be useful in a form script, for use with #system, for example.
@jodavies
Copy link
Collaborator Author

jodavies commented Mar 6, 2023

This causes a valgrind read error in vorm tests, I am unable to tell so far if that is my fault or of it simply uncovers something else. If I understand correctly, adding new pre-proc vars really is this simple, right?

@tueda
Copy link
Collaborator

tueda commented Mar 6, 2023

I think the paths AM.TempDir and AM.TempSortDir are not initialized at the code you inserted (they are modified below).

Edit: Never mind. I was wrong.

@jodavies
Copy link
Collaborator Author

jodavies commented Mar 6, 2023

I can reproduce this in my machine if I copy the code of the failing test. However if I modify it by, say, defining two more pre-proc variables in the form script which don't do anything, valgrind is again happy.

@tueda
Copy link
Collaborator

tueda commented Mar 6, 2023

OK, this is not your fault. Actually, the following code gives a Valgrind error with the master branch:

#do i=1,5
  #define x`i'
#enddo
#define var1(a) "(`~a')"
#message `var1(1)'
.end

Something wrong seems to occur for FromList and defining the macro with the deferred substitution...

Edit: deferred substitution is not needed to reproduce the Valgrind error:

#do i=1,5
  #define x`i'
#enddo
#define var(a) "(b)"
#message `var(1)'
.end

@tueda
Copy link
Collaborator

tueda commented Mar 7, 2023

The memory error was fixed in 741861a.

I am not sure about the use case of these variables with #system or other scriptings. Basically, temporary files in TempDir/TempSortDir should not be touched, right? Or do you need just the directory paths, say, to get a temporary directory? (But in this case, you can use FORMPATH or TMPDIR, which are probably defined in your environment.)

@jodavies
Copy link
Collaborator Author

jodavies commented Mar 7, 2023

I would like to do something like

#system gunzip < /network/path/large.sav.gz > `FORMTMP_'/large.sav
Load `FORMTMP_'/large.sav;
#system rm `FORMTMP_'/large.sav

Personally I have FORMTMP defined in my environment, but this doesn't work if a user has defined their temp dir with the -t argument instead.

@tueda
Copy link
Collaborator

tueda commented Mar 7, 2023

On the one hand I'm still not so convinced why `TMPDIR'/large.sav is not enough for temporary files (OK, maybe it depends on cluster configurations), but on the other hand invoking form -t /tmp prog.frm would be handier than FORMTMP=/tmp form prog.frm for such scripting...

I would merge this pull request, but before that, does anyone else have any other opinions?

@jodavies
Copy link
Collaborator Author

jodavies commented Mar 7, 2023

In the environment variable case you need to have called form as form -d TMPDIR=$FORMTMP anyway don't you? In the event that you don't have full control over how your form code is called, it seems to me this is the only way to guarantee access to the temp directory in the script.

@tueda
Copy link
Collaborator

tueda commented Mar 7, 2023

In the environment variable case you need to have called form as form -d TMPDIR=$FORMTMP anyway don't you?

Well, yes, if TMPDIR must have the same value as FORMTMP and so you want to overwrite the default TMPDIR with FORMTMP.

I assume the environment variable TMPDIR is defined on Unix or at least on Linux, so it is available for the preprocessor. For Windows or other special environments, #ifdef is needed to check if TMPDIR is given by the system. I think Windows provides TMP and TEMP instead of TMPDIR, so a generic way would be:

#if (isdefined(TMPDIR))
  #define mytempdir "`TMPDIR'"
#elseif (isdefined(TMP))
  #define mytempdir "`TMP'"
#elseif (isdefined(TEMP))
  #define mytempdir "`TEMP'"
#else
  #define mytempdir "."
#endif

#message `mytempdir'/large.sav
.end

To me, TMPDIR sounds more or less for temporary files in general, and FORMTMP is for temporary files that FORM automatically creates (I admit that you may argue that FORMTMP could be for any temporary files during a FORM run, though).

In general, TMPDIR may differ from FORMTMP (though you may set export FORMTMP=$TMPDIR on your shell). Whether it is better to make temporary files in TMPDIR or FORMTMP (speed/storage size) depends on where FORM is running. In this sense, the script needs to know the environment and has no way to determine where is the best place for temporary files. The script needs some assumption for it or the user who invokes FORM needs to specify this information in some way (which may be by the -t option as you mentioned).

@jodavies
Copy link
Collaborator Author

jodavies commented Mar 7, 2023

I didn't actually know that the preprocessor will search for variables in the environment if they are not defined in the script or as an argument, thanks.

For my purposes, what would actually be even cleaner is if FORM were to compress the expression data inside the save files by itself. Usually I see something like a 10x compression ratio here, which saves quite some disk space and network bandwidth. This is much more difficult to implement, however.

@vermaseren
Copy link
Owner

vermaseren commented Mar 7, 2023 via email

@tueda
Copy link
Collaborator

tueda commented Mar 8, 2023

Probably, saving files with gzip-compressed expressions would be discussed as a separate issue. I have created it: #436.

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.

3 participants