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

support to access local properties via propertyset #126

Closed
wants to merge 1 commit into from

Conversation

babasaikiran
Copy link

The fix here is to honor the local variables in propertyset within the local scope. Below is the bug related to the fix.

Bug 50179 - Properties declared as "local" are not accessible via propertyset.

@jaikiran
Copy link
Member

this is ok to test

@babasaikiran
Copy link
Author

@jaikiran Any update on this ?

@bodewig
Copy link
Member

bodewig commented Jul 29, 2020

unfortunately with this PR the testGlobalLocal AntUnit test fails:

<target name="testGlobalLocal">

@babasaikiran
Copy link
Author

@bodewig , had a look at the fail case.. seems this particular testcase is failing only on windows environment.

Will try to get a windows machine to try and fix it.

@bodewig
Copy link
Member

bodewig commented Jul 29, 2020

It failed on my Linux box (Ubuntu) using OpenJDK 8.

@babasaikiran
Copy link
Author

I am on Linux mint 20 and with jdk1.8.0_181.
Executed the test case with commnad: ./build.sh antunit-tests -Dantunit.testcase=**/local-test.xml.

here is the output towards end:

antunit-tests:
[au:antunit] Build File: /home/sai/open/ant/src/tests/antunit/taskdefs/local-test.xml
[au:antunit] Tests run: 6, Failures: 0, Errors: 0, Time elapsed: 2.162 sec
[au:antunit] Target: testMacrodef took 0.012 sec
[au:antunit] Target: testSequential took 0.004 sec
[au:antunit] Target: testTarget took 0.003 sec
[au:antunit] Target: testGlobalLocal took 0.001 sec
[au:antunit] Target: testParallel took 2.037 sec
[au:antunit] Target: testBaseline took 0.001 sec

Please let me know.. if i am missing or doing something wrongly.

@bodewig
Copy link
Member

bodewig commented Jul 30, 2020

It's been several years since I last looked at the implementation of local properties, something I need to change before I can merge your PR anyway. Right now I'm not sure of all the implications of your change to LocalPropertyStack.

stefan@numpad:~/devel/ASF/ant$ uname -a
Linux numpad 5.4.0-29-generic #33-Ubuntu SMP Wed Apr 29 14:32:27 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
stefan@numpad:~/devel/ASF/ant$ java -fullversion
openjdk full version "1.8.0_252-8u252-b09-1ubuntu1-b09"
stefan@numpad:~/devel/ASF/ant$ git checkout -b babasaikiran-bug-50179 master
Zu neuem Branch 'babasaikiran-bug-50179' gewechselt
stefan@numpad:~/devel/ASF/ant$ git pull https://github.com/babasaikiran/ant.git bug-50179
Von https://github.com/babasaikiran/ant
 * branch                bug-50179  -> FETCH_HEAD
error: Terminal is dumb, but EDITOR unset
Merge wurde nicht committet; benutzen Sie 'git commit', um den Merge abzuschließen.
stefan@numpad:~/devel/ASF/ant$ ./bootstrap.sh 
... Bootstrapping Ant Distribution
...
BUILD SUCCESSFUL
Total time: 6 seconds
... Cleaning Up Build Directories
... Done Bootstrapping Ant Distribution
stefan@numpad:~/devel/ASF/ant$ ./build.sh antunit-tests -Dantunit.testcase=**/local-test.xml
Buildfile: /home/stefan/devel/ASF/ant/build.xml
...
antunit-tests:
Created dir: /home/stefan/devel/ASF/ant/build/antunit/xml
Build File: /home/stefan/devel/ASF/ant/src/tests/antunit/taskdefs/local-test.xml
Tests run: 6, Failures: 1, Errors: 0, Time elapsed: 2,079 sec
Target: testMacrodef took 0,005 sec
Target: testSequential took 0,001 sec
Target: testTarget took 0 sec
Target: testGlobalLocal  FAILED
	at line 28, column 21
	Message: Assertion failed
	took 0,002 sec
Target: testParallel took 2,019 sec
Target: testBaseline took 0 sec

BUILD SUCCESSFUL
Total time: 8 seconds

@babasaikiran
Copy link
Author

Thanks @bodewig , ./bootstrap.sh is the missing piece and now i am able to reproduce it.

@bodewig
Copy link
Member

bodewig commented Aug 2, 2020

By returning false from LocalPropertyStack's setNew you create a copy of the local property inside of the project properties themselves. I can see you do this because there is no way to enumerate all properties that are in scope - there are a few comments in PropertyHelper that indicate we have been aware of the problem back when we worked on Ant 1.8.0.

What would probably be cleaner was to finally introduce a new PropertyEnumerator delegate along with new methods left and right and allow PropertySet to use that. I'll sketch this in a branch.

@bodewig bodewig mentioned this pull request Aug 2, 2020
@bodewig
Copy link
Member

bodewig commented Aug 2, 2020

@babasaikiran please take a look at #135 for my suggestion for fixing the bug

@babasaikiran
Copy link
Author

@bodewig Tested the #135 and things are working as expected.

@bodewig
Copy link
Member

bodewig commented Aug 11, 2020

replaced with #135

@bodewig bodewig closed this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants