Small changes to thisREST.cmake / thisREST.sh

[lobis]

Just some small changes.

• Use $REST_PATH when available
• Do not include Garfield in $ROOT_INCLUDE_PATH (ill formatted) if Garfield is not installed.

[juanangp]

Suggested change

	export ROOT_INCLUDE_PATH=\\\$REST_PATH/include${Garfield_INCLUDE_ENV}:\\\$ROOT_INCLUDE_PATH
	export ROOT_INCLUDE_PATH=\\\$REST_PATH/include:\\\$ROOT_INCLUDE_PATH
	export ROOT_INCLUDE_PATH=\\\${Garfield_INCLUDE_ENV}:\\\$ROOT_INCLUDE_PATH

[lobis]

if we add a : then when Garfield is not installed it will be two ::. The variable itself has the :. (this suggestion should not be commited).

[jgalan]

It is strange that the Path contains already the :

[jgalan]

I see, ok, there is a problem to have :: inside?

[lobis]

I see, ok, there is a problem to have :: inside?

no, but why do we want double :: when we can just put a single :?

[lobis]

But currently we have export ROOT_INCLUDE_PATH=\\\$REST_PATH/include${Garfield_INCLUDE_ENV}:\\\$ROOT_INCLUDE_PATH

As far as I understand this is not a valid path

Besides, I found misleading to add these paths to inside ROOT_INCLUDE_PATH

why is this not a valid path? it works when Garfield is instaleld or not.

If garfield is installed it expands to:

export ROOT_INCLUDE_PATH=$REST_PATH/include:/usr/local/garfieldpp/install/include:$ROOT_INCLUDE_PATH

If its not:

export ROOT_INCLUDE_PATH=$REST_PATH/include:$ROOT_INCLUDE_PATH

[juanangp]

I see, I oversee this path set(Garfield_INCLUDE_ENV ":$ENV{GARFIELD_INSTALL}/include")

For me is misleading to add : inside Garfield_INCLUDE_ENV

I believe should be someting that:
set(Garfield_INCLUDE_ENV "$ENV{GARFIELD_INSTALL}/include")

export REST_INCLUDE_PATH=\\\$REST_PATH/include
export REST_INCLUDE_PATH=\\\$ROOT_INCLUDE_PATH:\\\$ REST_INCLUDE_PATH
export REST_INCLUDE_PATH=\\\$Garfield_INCLUDE_ENV:\\\$ REST_INCLUDE_PATH

[jgalan]

Yes, but here is a bit different, because sometimes we do not want to add the PATH, to do it the way you mention we should add an if condition somewhere.

[lobis]

I see, I oversee this path set(Garfield_INCLUDE_ENV ":$ENV{GARFIELD_INSTALL}/include")

For me is misleading to add : inside Garfield_INCLUDE_ENV

I believe should be someting that: set(Garfield_INCLUDE_ENV "$ENV{GARFIELD_INSTALL}/include")

export REST_INCLUDE_PATH=\\\$REST_PATH/include
export REST_INCLUDE_PATH=\\\$ROOT_INCLUDE_PATH:\\\$ REST_INCLUDE_PATH
export REST_INCLUDE_PATH=\\\$Garfield_INCLUDE_ENV:\\\$ REST_INCLUDE_PATH

Then we will have double : on the final thisREST.sh when it could have been avoided by including the : in the env variable. I prefer my solution since it produces the cleanest result at the expense of a misleading : in a cmake variable that nobody will access anyway, but I don't really care tbh. What do you think @jgalan ?

[jgalan]

Anyhow I think it can be approved without major issue as it is