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 win datadir #22

Merged
merged 7 commits into from Sep 16, 2021
Merged

Fix win datadir #22

merged 7 commits into from Sep 16, 2021

Conversation

fredrikw
Copy link
Contributor

@fredrikw fredrikw commented Sep 9, 2021

This is intended to fix the issues that arises due to Conda not able to find the correct BABEL_DATADIR when building on Windows. This is the reason behind openbabel/openbabel#2409 and openbabel/openbabel#2372

I also tried to change the testing to actually fail when tests fail. Since obabel exits with 0 also when formats or data files are not found the current tests don't amount to anything. There are optional ways to do this but this was most in line with the current solution I think. An option would be to change the tests: -> commands-lines to things like - python -c "import subprocess;exit(b'1 molecule converted' not in subprocess.run(['obabel', '-:c1ccccc1', '--gen3d', '-oinchi'], capture_output=True).stderr)" but I thought the way in this PR is more clear.

It's marked as WIP since I wanted to get a failing test on the CI before correcting the issue. I also have a question on the best possible solution to set the env correctly that I will expand on below in comments. I will also take care of the build number and re-rendering before removing WIP

Checklist

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@fredrikw
Copy link
Contributor Author

fredrikw commented Sep 9, 2021

Ok, now there is a fix pushed that fixes all the failing tests. This solution is the simplest one, where we copy a script to set the environment variable on activation of the conda env. According to conda-build docs (https://docs.conda.io/projects/conda-build/en/latest/resources/activate-scripts.html) this is to be avoided when possible but I don't understand the build process well enough to find another solution. It should be possible to update the build files to have the correct hard-coded BABEL_DATADIR (as it does for OSX and Linux) but my efforts in that direction has failed.

I'll leave it like this for a couple of days to see if anyone has a hint as to how we could make a less hacky solution.

@njzjz
Copy link
Member

njzjz commented Sep 9, 2021

@conda-forge-admin please rerender

@github-actions
Copy link

github-actions bot commented Sep 9, 2021

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@fredrikw fredrikw changed the title [WIP] Fix win datadir Fix win datadir Sep 15, 2021
@fredrikw
Copy link
Contributor Author

With no other input, I went ahead and bumped the build number and removed the WIP-status. I also updated the activation-scripts to stash and reset BABEL_DATADIR if it was set before activation of the conda environment.

To me, this is ready to merge now.

@mcs07
Copy link
Contributor

mcs07 commented Sep 15, 2021

I haven't been able to test locally, but in principle this looks fine to me.

Ideally the hard-coded path detection and prefix replacement in conda-build would mean this isn't necessary... but I don't know enough about it to say whether it is possible to rely on that. Maybe if the BABEL_DATADIR environment variable is set at build time it might help?

Also, I take it that there's no need to set BABEL_LIBDIR in the same way?

@fredrikw
Copy link
Contributor Author

I also thought the path-detection and prefix replacement would do it, but I couldn't make it work. I managed to get the path updated in the binary files, but there is something in how BABEL_DATADIR works that still made the wrong folder turn up in tokenst.cpp from the makro. Too much c++-makro smartness for my brain...
I haven't noticed a problem with BABEL_LIBDIR, not for myself and not in user reports.

@baoilleach
Copy link

Thanks for all your work here. We really should move away from relying on BABEL_DATADIR to be set. With OB 3.0, I managed to reduce the amount of data read from there but there's still more to go.

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.

None yet

5 participants