Skip to content

Conversation

@mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Dec 12, 2018

Purpose

@QilongTang noticed a change in the libG folder names copied the bin directory.
I'm not a fan of the libG folders in extern being LibG_ and those in bin being libG_ - but I think this is the least risky change at this point.

Normally this should not be a problem on standard windows, but it might be on mono, mac, linux platforms.

This PR just reverts the copy steps to their previous behavior.

Some alternatives:

  1. change all the libG loading to use uppercase paths like
    var libGFolderName = string.Format("Libg_{0}_{1}_{2}", major, minor, build); instead of the lowercase libG.
  2. change the folders in extern to be lowerCase and request BRE team to change their names in autogenerated cross merge PR.
  3. try to normalize the paths before checking if they exist on disk, using toLower() or some other mechanism - seems like the most work.
  4. do nothing - likely will cause problems on other platforms or windows machines that have case sensitive filesystems enabled.

Looking for some feedback/thoughts.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@QilongTang
@aparajit-pratap

FYIs

@smangarole

@mjkkirschner mjkkirschner changed the title Asm225 libG copy steps should have the same behavior as before. Dec 12, 2018
@QilongTang
Copy link
Contributor

@mjkkirschner Let's address this case sensitive thing when we move to nugets, the fix looks good to me.

@aparajit-pratap
Copy link
Contributor

@mjkkirschner does this pose an issue in Windows? How and when does it surface?

@QilongTang
Copy link
Contributor

QilongTang commented Dec 12, 2018

@aparajit-pratap I was testing ASM 225 on my machine locally and loadling libG failed for me because of case sensitive setting on my machine
My local failure is due to another error, but the problem is still valid

@mjkkirschner
Copy link
Member Author

@aparajit-pratap on windows this will cause issues if a user has enabled file system case sensitivity which is now an option in windows 10. 😬

@mjkkirschner mjkkirschner merged commit 7ab5632 into DynamoDS:master Dec 13, 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.

3 participants