Changing the library used to read the properties files

[haroon-sheikh]

Signed-off-by: Haroon [email protected]

Problem:
If a properties file has a lot of properties split into different sections by comments. E.g.

# ------------ Section 1 ------------------
prop=...
..
# ------------ Section 2 ------------------
# property comments
prop2=...

Then all of the properties below including prop2 is being skipped by the old properties reader and not set as env vars.

The dmotylev/goproperties library hasn't had any update since 2014 so it would be a good idea to move onto something like magiconair/properties which is more widely used.

Fixes #1734

[sriv]

@haroon-sheikh thanks, this is indeed worth changing.

Some builds are failing, let me know if you need any help in sorting these out.

[haroon-sheikh]

@haroon-sheikh thanks, this is indeed worth changing.

Some builds are failing, let me know if you need any help in sorting these out.

Yes please, I'm not sure why the tests are failing. All of them passing when I run it locally. Is there something environmental I need to do?

[haroon-sheikh]

Should be okay now @sriv, can you review for me please?

[github-actions]

Benchmark Results

java_simple_serial.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	179%	351068	0:13.29	0
c532320	170%	343216	0:14.42	0
c93f7af	163%	341344	0:15.11	0
3d3f0af	170%	326256	0:18.09	0

java_gradle_serial.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	179%	388428	0:21.85	0
c532320	177%	349640	0:21.11	0
c93f7af	166%	619168	0:33.39	0
3d3f0af	184%	560128	0:35.55	0

java_simple_parallel.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	185%	316576	0:20.36	0
c532320	188%	316540	0:20.17	0
c93f7af	182%	324672	0:20.33	0
3d3f0af	181%	399160	0:19.93	0

java_maven_multithreaded.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	183%	355184	0:23.08	0
c532320	183%	303728	0:22.61	0
c93f7af	153%	362648	0:37.51	0
3d3f0af	168%	281916	0:26.06	0

java_gradle_parallel.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	178%	394976	0:20.54	0
c532320	178%	374068	0:20.26	0
c93f7af	166%	639544	0:30.80	0
3d3f0af	184%	601508	0:40.24	0

java_maven_serial.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	181%	318024	0:17.65	0
c532320	181%	338348	0:18.11	0
c93f7af	147%	296200	0:30.25	0
3d3f0af	153%	311920	0:28.48	0

java_simple_multithreaded.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	181%	397856	0:13.15	0
c532320	174%	384524	0:13.73	0
c93f7af	174%	385536	0:14.13	0
3d3f0af	164%	353616	0:15.12	0

java_gradle_multithreaded.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	180%	376804	0:18.33	0
c532320	182%	355644	0:20.04	0
c93f7af	179%	558492	0:33.83	0
3d3f0af	177%	595976	0:30.46	0

java_maven_parallel.csv

Commit	CPU	Memory	Time	ExitCode
6e0d83f	184%	318832	0:21.98	0
c532320	184%	340820	0:20.92	0
c93f7af	158%	341744	0:31.93	0
3d3f0af	154%	326644	0:30.28	0

Notes

• The results above are generated by running against seed projects in https://github.com/getgauge/gauge-benchmark
• These results are not persisted, but on merging to master the benchmark will be rerun.
• These benchmark are run in Github Actions' agents, which are virtualized. Results are not to be taken as actual benchmarks.Rather, these are indicative numbers and make sense for comparison.

---

See Workflow log for more details.

[sriv]

hi @haroon-sheikh - the change makes it better, but I still feel that master has more details in the error message.

ex: with

foo = ${bar}
bar = ${foo}

master shows:

circular reference found in env variable 'foo'. Check for cyclic property definitions in:
foo=${bar}
bar=${foo}

whereas this change shows:

Failed to load env. Failed to parse: /tmp/foo/env/default/default.properties. circular reference in "bar = ${bar}"

The cyclic graph is missing, and I worry that it can be hard to locate it when it happens.

[haroon-sheikh]

@sriv Thanks for reviewing and I completely agree with you. I'm now thinking if we should actually do this update. I was looking to implement a simpler solution so that we remove custom code but it looks like with our requirements, it's required to implement our own version of properties module. Shall we leave it as is for now?

[sriv]

I think we should address the fact that the current dependency is not maintained. And I appreciate all the work that has gone into this PR.

I'll take a shot at looking at some way to get the cyclic graph in. Since this fixes #1734 (which I believe is more important than the cyclic deps), I'd like this to be pursued (and am happy to help get this in).

All this deliberation is just because this is sort of regressing :(

[haroon-sheikh]

Yes please, I'm happy for you to takeover from here to fix the regression if you've got some time.

[haroon-sheikh]

I've pushed another failing test if you can have a look at that also please.

[sriv]

hi @haroon-sheikh I finally got to spend some more time on this. I felt the best place for this change would still be in the upstream, so I've raised another pull request: magiconair/properties#50

[sriv]

@haroon-sheikh magiconair/properties#50 is now merged and released. Can you please try updating the dep?

Also, note instead of properties.LoadFile gauge could probably benefit better with properties.MustLoadFiles which could load all the properties files in order. This would mean more refactoring, so if you feel we could take this up as a separate task, I can understand.

[haroon-sheikh]

Thanks @sriv. I've now updated the lib and failing unit tests. There's a unit test scenario TestLoadDefaultEnvWithMissingSubstitutedVariableAcrossMultipleFiles which I'm not able to fix, would you be able to give me a hand with this?

[sriv]

Thanks @haroon-sheikh . @BugDiver has accepted to look into this further.

[BugDiver]

@haroon-sheikh I looked into the PR. The failing test is because of the way we are substituting the env. We parse the env ${a} and set a as key and ${a} as it's the default value which causes the circular ref, but we need to do it because it can refer to other envs as well.

To fix it I tried using MustLoadFiles which loads all the files (we do not have to loop over files as @sriv was suggesting),. This makes the test pass, and the output is also right. The drawback is this function panic on errors, and the error message contains timestamps (because of panic I guess).

IMO library method should return an error instead of panicking. I trying to find a way if I can recover from panic and create a proper error message.

[haroon-sheikh]

Thanks for looking into this @BugDiver. I think one of the problems in the new lib is that the files take priority over env vars. And we want env vars to take precedence instead. Do you think it's worth us creating our own properties module in the project (or is it an overkill)? This can then later be shared in other projects. That would let us define the logic in the way we want.

[BugDiver]

I think one of the problems in the new lib is that the files take priority over env vars.

True

Do you think it's worth us creating our own properties module in the project (or is it an overkill)?

Not sure if it will be overkill. But I think it's quite an effort

Also since this lib panic on errors, it's hard for us to give proper error messages.

We can either submit patches to the lib or fork it and change it according to our requirements.
If we want to maintain our own, I guess we could just fork the older one or the new one and make changes.

[BugDiver]

@haroon-sheikh I have pushed a few changes to the env branch. Please have a look at it, try to install gauge from this branch, and check if all the cases are covered and it's giving the desired output/error. Also Please check the tests as well

[haroon-sheikh]

Thanks @BugDiver. I've had a look at the env branch and it seems to do the job and all of the tests are passing too.

[BugDiver]

@haroon-sheikh Thanks for the quick response. If you want you can rebase your PR with the branch, or I can raise a PR from that branch

[haroon-sheikh]

Please raise another PR and I'll close this one down. ;)