-
-
Notifications
You must be signed in to change notification settings - Fork 35
Check availabilty of all depedencies when building if necessary #1946
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
base: master
Are you sure you want to change the base?
Conversation
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.
OK... obviously I like your approach! Good job! 😄
I have a suggestion:
Can we somehow do it by OS/distro?
I'm not saying that you have to add the dependencies for each and every one of the systems yourself, but 💡 you could actually write/add the basic structure so that we can keep adding dependencies for new OSes.
To begin with, the dependencies
table could be a table with the OSes/distros as keys and then the different dependencies as a sub-table.
As for the checking (in checkDependencies
) I believe we can partly use Nim's own OS checking mechanisms (e.g. when defined(Linux) ....
, etc) along with some custom distro-spotting code perhaps(?).
What I mean (obviously, the code below is off the of my head - "extremely draft-ish" warning ❗ lol):
...
var deps: Table[String,String] = {}.toTable
when defined(Linux):
if distroIsFedora(): # I have no idea how this can be checked
# but there must be a way
deps = dependencies["Fedora"]
else:
discard # for now
else:
discard # for now
for dep in deps:
# ...
Obviously, it can get even trickier along the way (e.g. we can't just use pkg-config
on all systems - we have to even adapt our dependency checks), but I think we could make it a bit more flexible?
What do you think?
I agree with your comments. |
I think it's in good shape, BSDs have checks bypassed for now, but the data structures already have the scaffolding needed to finish it in the future if needed. |
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.
Well, looks good to me!
I've checked each review I'd made, and you managed all of them correctly.
I just added a review comment about a typo, but this is not that important, I can fix it myself.
Also, I tested locally on my machine. Obviously, I could just cover the Windows version, but ohh, well...
If CI passes and @drkameleon approves, all set up and ready 🙂!
Description
This PR checks that all required dependencies are available when building with
--mode full
or--mode docgen
.This is done before any building or compilation.
Advantages
Disadvantages
Todo
This only works on Linux, I have not tested or written this to work on Windows or MacOs.
Future Directions
It would be nice to make sure this system works on all major distros and that package names in the error message were applicable to most repositories, for now I just used the names correct for Red Hat/Fedora/Rocky Linux etc.
Should this problem be brute-forced by having a table like this?
and then change:
to:
Type of change