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

Fix to windows setenv() which would strip single backslashes #27

Merged
merged 1 commit into from Aug 17, 2017

Conversation

Projects
None yet
2 participants
@jonathon-love
Copy link
Contributor

jonathon-love commented Aug 16, 2017

hi dirk,

i found that setenv() from setenv.c would perform the following modifications to the variable:

C:\Users\\Jonathon\\\Documents\\\\Fred -> C:Users\Jonathon\\Documents\\\Fred

where as i believe a better behaviour would be:

C:\Users\\Jonathon\\\Documents\\\\Fred -> C:\Users\Jonathon\\Documents\\Fred

(the original behaviour resulted in an R_SESSION_TMPDIR on my machine of C:UsersJonathonAppDataLocalTemp)

there was a mixture of tabs and spaces in setenv.c, so i corrected that in the first commit, where as the second commit contains my proposed changes.

with thanks

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 16, 2017

What is your suggested change? I cannot see it among 50 or lines of whitespace change.

If there is possible issue (which I don't doubt; I prefer forward slashes whereever possible) could you construct a unit test showing that it fails before your change and not after?

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 16, 2017

What is your suggested change? I cannot see it among 50 or lines of whitespace change.

hi dirk,

i split it into 2 commits. whitespace changes in the first, actual changes in the second. here's the second:

46984e4

If there is possible issue (which I don't doubt; I prefer forward slashes whereever possible) could you construct a unit test showing that it fails before your change and not after?

i can convert all the backslashes into forward slashes, but i was being more conservative. it's possible environmental variables may contain a backslash which isn't part of a path. but i can take a more heavy handed approach (collapsing all backslashes into a forward slash) if you prefer.

with thanks

jonathon

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 16, 2017

Ok, that second commit is clearer. Despite the few coffees I had by now, and the run earlier which should have cleared my head, I am not fully able to assess it as C-level string hacking is ... fugly.

So two things:

-- the unit test, please?
-- can you revert the seemingly content-free whitespace change?

If git frustrates you and you can't, no worries, we survive, but strictly speaking it dies not belong here. But a test would be good. Also, entries in ChangeLog and NEWS.Rd that follow the normal conventions there would be nice, but I am happy to do that later at release times.

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 16, 2017

-- the unit test, please?

can do ... but i'm not quite sure where to put unit tests in RInside. i see that it has a .travis.yml file, but i can't see any tests that actually get run. the other issue is that this setenv.c is only used on windows, so this won't show up on travis any way.

any guidance?

-- can you revert the seemingly content-free whitespace change?

wait? talk me through this. i can certainly get rid of that commit, but i thought fixing whitespace was one of the few things in programming that everyone agreed on :P

with thanks

jonathon

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 16, 2017

Touche. There are currently no unit tests.

Can you mock up something ("anything") I can run once I fire up a VM with windoze?

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 16, 2017

crikey. i guess, but it will take me some time.

cheers

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 16, 2017

Maybe a simple file for sourceCpp() illustrating what is passed to setenv, before and after?

@jonathon-love jonathon-love force-pushed the jonathon-love:master branch 6 times, most recently from 84b69af to 05a1aaa Aug 16, 2017

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 16, 2017

hi dirk,

i have ammended the commits. the first commit is adding a test, and the second is the fix to make the test pass.

(the tests work fine in my VMs, but i'm unsure what the issue is with travis. it looks like you have your own test system. is there a trick to making sourceCpp() work with it? build-essential is installed, so it should work)

with thanks

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 17, 2017

Let's backtrack.

We are certainly not adding testthat to the dependencies if the package had no tests so far (as I acknoweldged earlier).

Provide a free standing test script I can run. Remove anything intrusive from the PR. We take it from there.

@jonathon-love jonathon-love force-pushed the jonathon-love:master branch from 05a1aaa to d5925f0 Aug 17, 2017

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 17, 2017

hi dirk,

i've stripped back the PR, and here is a script to check my changes to setenv.c:

https://jona.thon.love/test_setenv.R

with thanks

jonathon

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 17, 2017

Much appreciated. Will try to make some time to fire up the VM and check, but I have no reason to doubt it works.

So you are up and running locally but would be helped with an updated new release, I presume?

@jonathon-love

This comment has been minimized.

Copy link
Contributor Author

jonathon-love commented Aug 17, 2017

Much appreciated. Will try to make some time to fire up the VM and check, but I have no reason to doubt it works.

i think the test will work on any OS ... in practice, RInside only uses it on windows, but there's no platform specific code in there.

So you are up and running locally but would be helped with an updated new release, I presume?

it's actually not a huge deal for us. we ship as a monolithic application, and we statically link against RInside. if other people were building jamovi on windows, then an update would be helpful, but building jamovi on windows is so insanely complex i think we're the only ones doing it.

an updated version on CRAN will of course be helpful in the long term, but in the short-term it's not really an issue.

cheers

@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 17, 2017

Thanks for clarifying; I was of course of the #ifdef-ness on your change and its "Windows-only" character. It really seems like there is no rush besides getting the master branch updated.

@eddelbuettel eddelbuettel merged commit 6c08434 into eddelbuettel:master Aug 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Owner

eddelbuettel commented Aug 17, 2017

All good. Thanks so much too for the test_setenv.R -- I made it test-setenv.cpp with embedded R code, and replace the spurious testthat use with plain old all.equal -- all good. Nice. test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.