-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Enable flook #50
Enable flook #50
Conversation
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 ( |
build-number update required, otherwise we can check it. |
You are too fast, I was doing it :) |
@conda-forge-admin, please rerender |
Is there some way to try with the current master of siesta? Or is it only possible with tagged releases? |
The MAC build seems to have gone over flook without problems, only the linux one has failed |
conda releases should stick to a formal release, IMHO. So I think you have these options:
From the looks of it, then it seems the problem will be present in later commits as well. It could be env-vars. Play with that. |
Yes, I just wanted to try here. What you build here in the PR doesn't get published, does it? |
No, only once merged. :) |
Do you think this:
Means that gcc is not available, or is it an error of gcc not finding some file? 😅 |
typically conda gcc on linux is named very weirdly, look through the look files for GCC_CC or similar, then you'll find long names. :) |
@conda-forge-admin, please rerender |
Not pretty, but it works :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is too hacky, could you try with fixing the setup.make instead?
recipe/build.sh
Outdated
@@ -64,13 +64,29 @@ else | |||
MPI=ON | |||
fi | |||
|
|||
if [[ "$(uname)" == "Linux" ]]; then | |||
# Flook compilation on linux needs some hacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. Very non-standard.
I would suggest you fix the setup.make
input file instead, via a patch.
This should also ensure that readline is captured correctly.
recipe/meta.yaml
Outdated
@@ -44,6 +44,7 @@ requirements: | |||
- git | |||
- cmake | |||
- pkg-config | |||
- readline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readline shouldn't be in build
section.
I have the impression that The CMake_*_compiler are defined here: But only the fortran ones are set. I looked through the CMakeLists file and I can't find where |
I think you should just patch |
Isn't that equally hacky? I think this is a problem for any SIESTA compilation, not just the conda one. It only works if there is by chance a |
No, not really. The patch should point the CC to the correct GCC variable. In this case it is how conda-forge setups its build procedure. And that may be very different from local installations. I think this is not a problem in the siesta repo as the cmake invocation correctly sets the CC variable in |
You can use:
For more explanations https://conda-forge.org/docs/maintainer/knowledge_base.html#using-cmake |
Ah, I always assumed CMAKE_ARGS was automatically read, then please try that here, yes! :) |
Ok, so I found out that
which exists, and it is a symbolic link to
So it is not a problem of cmake not finding the compiler. But somehow this is ignored and |
I think flook just ignores whatever you set up as C compiler in I tried with:
And I get:
So it uses gcc to compile C files but then it tries to use |
It also ignores the includes (I think it just ignores the recipe for So I think this is a problem that has to be solved in flook. Here it can only be solved in a hacky manner. |
Perhaps a way around this would be to: cd flook/aotus/external/lua
make CC=$GCC linux which should respect it, then a subsequent make shouldn't rebuild as it is already done. |
Shouldn't this be managed by flook? It will happen to all users trying to compile SIESTA, not just here in the conda compilation. Also the proposed workaround adds the extra requirement that flook must be pre checked out, it can't be downloaded on the fly by cmake during the compilation. |
Agreed, the problem is that Lua is compiled in
But I agree that a temporary commit could be done in flook |
I think not (I could be wrong though). |
So then, there could be a bash script or something? |
I think it is a matter of just cd'ing into |
Yes, but I think that is an odd requirement for users. Just as you cd into aotus here: Can't you cd into lua first? |
Yes, I am not talking about what we should do in flook, I am talking about what you should do, now, to fix conda build here. You still want to enable this at the current source tree of siesta+flook, which is hard-coded/fixed. So there is nothing to be done in flook, you have to work around that problem here. |
I can work around the problem by just creating a symbolic link If you agree that this is a bug and could be fixed in flook (and it seems like an easy fix), I don't see why we need to make things complicated (and waste time waiting for the very slow build feedback loop). |
I don't like doing symbolic links in conda-builds. I am worried it has side-effects we are not aware of. That is why I suggest you to change these things in this build procedure. Otherwise we should wait until 5.0.0-rc2 is released. Or even a later version. I don't have time this week (likely not before February) to fix stuff in aotus/flook. Again, a new fix in flook won't be used in this build as the version is fixed from the 5.0.0-rc1, as it should be. |
The C compiler that cmake uses (and that should be passed to lua make) is already a symbolic link, |
I'm afraid if I try to do it in the other way I will spend the whole day looking at logs and figuring out details with one iteration every 5 minutes, and it is very discouraging. But whatever, I can try it again when I have nothing to do. |
I think you have somewhat misunderstood what I have tried to convey... I don't really have much time now, so if you could help with the below things it would be much simpler. In flook-repo
In the siesta-feedstock-repo
|
2a357c3
to
d3c1273
Compare
@conda-forge-admin, please rerender |
5eb3791
to
40d4f42
Compare
Ok, so I have come up with a Makefile that works. However I don't think it should be used in general because it calls If I remove |
This is very confusing, the only build that fails is I need to publish a tutorial for SIESTA using |
a6734e1
to
b3becd4
Compare
So finally I took the approach of requiring The only extra thing to do is to tell As you can see it is all green 🟢 ✔️ So @albgar @jan-janssen @zerothi can any of you merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thing that needs changing is that the cmakelists.txt should be a patch file, not a copy file.
recipe/build.sh
Outdated
# include it in libflookall and we need to explicitly tell cmake | ||
# to find the lua library and link it. That's what we do in this modified | ||
# CMakeLists.txt that we copy to flook's directory. | ||
cp $RECIPE_DIR/flook_patches/CMakeLists.txt External/Lua-Engine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change the CMakeLists.txt
to a patch, instead of a full file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See on the 4.1 branch how to add a patch to the meta.yaml
file.
And remove the left over file. Then we should just wait for the green lights. |
ed6970a
to
5e8e3f9
Compare
All green 🟢 |
thanks!!! |
Now it failed in main due to a timeout 😅 |
annoying, and I can't get it to rerun until all the others have finished, basically the tests for this branch also required two re-runs... :( |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)