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

Fixes https://www.nsnam.org/bugzilla/show_bug.cgi?id=2098 #9

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

teto
Copy link
Member

@teto teto commented Aug 27, 2015

The documentation (https://www.nsnam.org/docs/dce/manual/html/dce-user-newapps.html) says
"Copy the executable file produced in a specified directory in the
variable environment DCE_PATH so that DCE can find it. (FIXME: to be
updated)" but wutils.py was overriding the user DCE_PATH and DCE_ROOT.

This patch prepends the user exported values for these 2 environment
variables.

@thehajime
Copy link
Member

I was planing to make those variables (i.e., DCE_ROOT, DCE_PATH) invisible because they sometimes made users confused.

I would like to know why those custom locations for dce binaries are useful: then this patch will get on the venue.

btw, if you would update wutils.py, you also need to update test.py as it also refers DCE_ROOT etc.

@teto
Copy link
Member Author

teto commented Aug 31, 2015

As you had advised me on the mailing list, I used to symlink my binaries in bin_dce but everytime I did a ./waf clean I had to redo the symlinks which is quite annoying since I see no way to automate this. Using DCE_PATH makes my programs loaded all the time and if I want to change the version of the program loaded, it's immediate to do $ DCE_PATH="new/path" ./waf --run and it keeps the default configuration.
If you are ok with this, I can also update test.py (or should test.py import wsutil.py ?)

@teto
Copy link
Member Author

teto commented Sep 3, 2015

My guess is that environments variables are often upper case and as the variable is presented in the documentation, people assumed it was an envionrment variable they could use as they use PATH. Problem is as it gets overriden, it didn't work hence the confusion. This patch would also clear the confusion I think.

@teto
Copy link
Member Author

teto commented Nov 2, 2015

I switched to the "master" branch for some testing and DCE could not find my binaries anymore. I updated the patch to modify test.py as well. Hope this can get merged.

@thehajime
Copy link
Member

thanks for the request.

  1. could you also propose a document update, i.e., to doc/source/dce-user-newapps.rst ?
  2. as ns-3-dev does, updating RELEASE_NOTES would be great. this should be in 'Bugs fixed' section in the top.

@teto teto force-pushed the fix_env_vars branch 3 times, most recently from cd527d1 to c5615ff Compare November 30, 2015 13:39
@teto
Copy link
Member Author

teto commented Nov 30, 2015

I don't know the .rst format so I kept the changes to minimal.

@thehajime
Copy link
Member

Thanks. one more (two?) thing.

  1. please rebase your tree to the master branch and 2) move the line of - Bug 2098.. to "Release dce-dev" in the RELEASE_NOTES.

The bugfix will be appeared in the next release.

@teto
Copy link
Member Author

teto commented Nov 30, 2015

done

@thehajime thehajime merged commit cae9dde into direct-code-execution:master Nov 30, 2015
@thehajime
Copy link
Member

thanks, applied.

The documentation (https://www.nsnam.org/docs/dce/manual/html/dce-user-newapps.html) says
"Copy the executable file produced in a specified directory in the
variable environment DCE_PATH so that DCE can find it. (FIXME: to be
updated)" but wutils.py was overriding the user DCE_PATH and DCE_ROOT.

This patch prepends the user exported values for these 2 environment
variables.
@teto teto mentioned this pull request Jan 15, 2016
@teto teto deleted the fix_env_vars branch March 16, 2016 15:17
@srathi1 srathi1 mentioned this pull request Jun 7, 2018
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.

2 participants