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

Make wrapper name unique using its source hash #117

Merged
merged 5 commits into from
Dec 23, 2019
Merged

Conversation

jneira
Copy link
Member

@jneira jneira commented Dec 20, 2019

  • Before this the wrapper used the project version to cache the wrapper executable

  • That led to false negatives testing the compilation and execution of the wrapper using the bios-test test suite cause the tests reused a previous wrapper executable between commits of the same version

    • Devs had to remember delete manually %XDG_DIRECTORY%\hie-bios to avoid it
  • I've used GHC.Fingerprint to hash the source file and cache the executable using its source (the most accurate one although non semantic changes will cause a cache miss)

    • And i've put the source near the executable to be able to check it quicky without have to go to hie-bios source
  • I've cherry-picked my changes onto 0.3.0 and 0.3.1 to check the test suite with them:

  • In both cases the test suite fails.

    • To be fair it had failed in the windows ci without these changes cause we are not caching %XDG_DIRECTORY%\hie-bios between ci builds and the wrapper would be compiled every time.
    • But if we start caching %XDG_DIRECTORY%\hie-bios in ci for some reason it will suffer the false negatives as well
  • I am using a new en var %HIE_BIOS_CACHE_DIR% to let users change the default one (without change the general %XDG_CACHEDIR%). It is not strictly necessary but it is a common practice to avoid issues with file permissions (in some envs the creation of executable files inside user directory is forbidden f.e.)

Fixes #114

@jneira jneira requested a review from fendor December 20, 2019 08:42
Comment on lines 79 to +84
getCacheDir :: FilePath -> IO FilePath
getCacheDir fp = getXdgDirectory XdgCache (cacheDir </> fp)
getCacheDir fp = do
mbEnvCacheDirectory <- lookupEnv "HIE_BIOS_CACHE_DIR"
cacheBaseDir <- maybe (getXdgDirectory XdgCache cacheDir) return
mbEnvCacheDirectory
return (cacheBaseDir </> fp)
Copy link
Collaborator

@fendor fendor Dec 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More Documentation for this function please!
Also, getCacheDir is probably a misnomer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm it retrieves the cache directory used by the lib, getHieBiosCacheDir would be better or you are thinking in another deficiency?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe a misnomer... Mainly adding documentation for the env variable and so on suffices for me.

@jneira jneira merged commit 90955b3 into haskell:master Dec 23, 2019
@jneira jneira deleted the wrapper branch December 23, 2019 20:10
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.

Make wrapper name unique
2 participants