AmpersandTarski / Ampersand

Build database applications faster than anyone else, and keep your data pollution free as a bonus.
http://ampersandtarski.github.io/
GNU General Public License v3.0
40 stars 8 forks source link

INCLUDE semantics contains an ambiguity #1428

Closed stefjoosten closed 10 months ago

stefjoosten commented 11 months ago

What happened

Consider the following script:

CONTEXT Issue1428
INCLUDE "../ArchiMetaModel.adl"

ENDCONTEXT

The file ArchiMetaModel.adl exists in the underlying directory and compiles independently without any error messages. However, I did an error message when compiling this with docker:

... (only showing the last few lines)

#0 15.64   While looking for /usr/local/ArchiMetaModel.adl
#0 15.64          File does not exist.
#0 15.64 2023-07-20 13:26:10.150131: [error] ExitFailure 10
------
failed to solve: process "/bin/sh -c ampersand proto /usr/local/project/Issue1428.adl   --proto-dir /var/www   --verbose" did not complete successfully: exit code: 10

What I expected

I expected compiling with a bare Ampersand compiler and the same compiler wrapped in Docker produces the same output.

Analysis

The explanation for this behavior is simple. The Ampersand compiler has no problems getting files from an underlying directory. However, there are concerns if this happens in an environment that is more complex than a single PC. If the result of an action (in this case: a compilation by Ampersand) depends on input from an underlying directory, the result will be different after the whole directory is moved to a different location. This also has security ramifications in general. That is why Docker does not allow looking in underlying directories.

Proposal

In contrast with my usual reflexes, I propose we forbid looking in the environment just like docker does. This means that the directories are interpreted from the root of the current working directory, just like Docker does. I think this restriction will prevent problems in the future, so it will help us to get a more reliable product. A time saver for the user will be that the Ampersand compiler produces the error message, instead of the environment, so we can make the error message more relevant from an Ampersand perspective.

Michiel-s commented 11 months ago

I understand the problem, but don't share the proposed solution.

Especially because we are thinking of replacing INCLUDE mechanism by a more modular like approach. I think it is worth spending the effort on that.

In the mean time, I know that the ../../ etc is used to have a set of shared scripts for multiple projects. E.g. the SIAM module. Preventing to go up the tree will make this use case impossible. Shared scripts need to be copied then.

Furthermore I think the error message is very good. It directly points out what is wrong and gives hint to the solution: copy those other files in your Dockerfile.