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

Support importing of Makefiles (as opposed to extending) #322

Closed
coffeenotfound opened this issue Nov 29, 2019 · 9 comments
Closed

Support importing of Makefiles (as opposed to extending) #322

coffeenotfound opened this issue Nov 29, 2019 · 9 comments
Assignees
Milestone

Comments

@coffeenotfound
Copy link

Features Description
This feature allows you to "import" another Makefile and depend on its tasks without extending it.

Describe the solution you'd like
Importing a makefile as opposed to extending it would allow depending on its tasks while still treating it as a seperate makefile. This is important for relative paths which break upon extending from a master makefile.

Makefiles would be imported under a given name in the imports table and it's (exported) tasks are then prefixed accordingly with the chosen name. (See example below)

Tasks should still be deduplicated across imported makefiles. E.g. in a diamond pattern of makefiles (A -> B -> D and A -> C -> D), a task in makefile D is only invoked once even if a task of B and one of C depend on it.

Tasks should also have a new boolean key "exported" (true by default) similar to "private" which controls whether the task is visible to importing makefiles.

Code Sample

[imports]
mybootloader = {path = "bootloader/Makefile.toml"}
mykernel = {path = "kernel/Makefile.toml"}

[tasks.make-all]
dependencies = [
    "build",
    "mybootloader.build",
    "mykernel.build"
]
@sagiegurari
Copy link
Owner

thanks for the feedback. i'm wondering what cargo-make provides now that can solve this and what needs development to help out making it simpler.
so... in your case, you could extend some other makefile and use/depend on the tasks there, but you are saying that relative paths are getting messed up as they are based on the current working directory and not makefile location.
so extended makefile having tasks with relative paths doesn't really know where its executed from.

to solve that, you have the possibility to define the current working directory on the task level and use cargo-make provided env vars and do relative paths from there.
for example:

[tasks.mybootloader-build]
cwd = "${CARGO_MAKE_WORKING_DIRECTORY}/somedir"
# or in case of a workspace you might want to use the workspace root as the root path:
# cwd = "${CARGO_MAKE_WORKSPACE_WORKING_DIRECTORY}/somedir"
command = "..."

I'm not entirely familiar with your use case so i'm wondering if this solves the issue for you?
or maybe we need to develop something to make it simpler?

@coffeenotfound
Copy link
Author

This works from the master makefile but it's a sub optimal solution as this breaks when using the nested makefiles directly. Eventhough there probably is a functional solution in this case, having to "work around" the limitations isn't very intuitive or enjoyable.

I believe my approach as outlined above adds a necessary primitive that has many more uses than my specific problems. If you think about it, extending and importing are two conceptually distinct operations that both have their use cases.

@sagiegurari
Copy link
Owner

I just want to understand the problem better before i change 30 files and we end up with same issue.
In this case, even import thing won't help you as commands are executed from current working directory. meaning relative paths are relative to working directory and not to makefile location.
With imports it would stay the same as tasks do not have the meta data of where they were defined. If for example I would add it, what would happen if a task was defined in 1 toml and then extended and modified in another which is in a different location? That would make the whole thing very complex to users to understand (and i assume imported tomls can also extend other tomls).

Therefore i'm trying to see what the best solution for your issue is, before jumping on the first one.
So in your case, you have a workspace, and sometimes you invoke it from workspace level and sometimes you invoke from module level.
And in both cases, you import some common makefile and invoke the tasks that are defined in that toml.
That common makefile has a task that invokes a command/script using relative paths and that they are relative to that common makefile.
(i hope i got your use case correct, an example would really help me).

So there are options today to resolve that, and they might be simple enough.
if they are not, lets see if we can come up with a simple solution (import as is, would result in same directory issue as tasks are using working dir not makefile dir).

option 1 - define new env var in makefiles and use in common as cwd

So you could have in workspace level and module level makefiles, in the env block a definition of that common working directory,

[env]
COMMON_WORKING_DIRECTORY="../../mydir"

naturally the value will be based on the module/workspace makefile you are editing.
In the common makefile you can have the cwd attribute using that env var defined in the caller makefile

[tasks.mybootloader-build]
cwd = "${COMMON_WORKING_DIRECTORY}"
# optionally you can define a condition to ensure that the env is set
condition = { env_set = [ "COMMON_WORKING_DIRECTORY" ] }
command = "..."

option 2 - wrap common task with local task with cwd

you can do the same trick by having some local task change the cwd and then call the common task with run_task.
the cwd would affect the common task as well

# module specific makefile
[tasks.mybootloader-build-wrapper]
cwd = "../../common"
run_task = "mybootloader-build"

and in common makefile you don't have to do anything at all.

I'm sure you won't have to do this for every task, only for the specific problematic task that has relative paths messed up.
If i had some examples i could understand more.

@coffeenotfound
Copy link
Author

Thanks for the answer!

That makefiles store metadata about where they are defined is pretty much the core of my idea. Instead of loading the different tomls and putting it all into one datastructure an imported makefile is it's own datastructure (which again could import or extend other makefiles). The only thing that connects them in the code is a reference from the importer to the importee.
All relative paths should be relative to the declaring makefile, too.

What this solves is the problem of nested projects which I think is a fairly common use case.
The solutions you provided may work (I couldn't actually get it to work, though) but they seem more like a workaround to me

@sagiegurari
Copy link
Owner

What if we store the file and directory that originally defined the task (ignoring the fact that it might be updated by extending files), then allowing you to do something like this:

[tasks.sometask]
cwd = { type = 'makefile' }

which you can set in specific tasks that need it in your common makefile.
you would still extend not import that file but it could give you the cwd dynamically without modifying every extending makefile.
would that solve it?

i'm against importing as it brings in a lot of complexity and confusion.
for example, if i import a makefile, do i have to load it right at the start? if it has an env block would it be loaded at the start? but what if i'm not using any of the tasks it holds?
that's just one example.

@coffeenotfound
Copy link
Author

I agree that import functionality would be kind complex to implement. Your idea seems like a good compromise. Having the cwd type as makefile (i.e. use the cwd of the declaring makefile) should be the default in my opinion (just like declaring an env var uses the cwd of the declaring makefile) but I guess that would break backwards compatibility

@sagiegurari sagiegurari added this to the 0.24.1 milestone Dec 5, 2019
sagiegurari added a commit that referenced this issue Dec 5, 2019
…EFILE, CARGO_MAKE_CURRENT_TASK_INITIAL_MAKEFILE_DIRECTORY environment variables #322
@sagiegurari
Copy link
Owner

@coffeenotfound just pushed a commit to the 0.24.1 development branch.
At the end, I decided to provide 3 new env vars that will be set at the start of every task invocation:

  • CARGO_MAKE_CURRENT_TASK_NAME - Holds the currently executed task name.
  • CARGO_MAKE_CURRENT_TASK_INITIAL_MAKEFILE - Holds the full path to the makefile which initially defined the currently executed task (not available for internal core tasks).
  • CARGO_MAKE_CURRENT_TASK_INITIAL_MAKEFILE_DIRECTORY - Holds the full path to the directory containing the makefile which initially defined the currently executed task (not available for internal core tasks).

This means, that in your common makefile, you can do the following:

[tasks.sometask]
cwd = "${CARGO_MAKE_CURRENT_TASK_INITIAL_MAKEFILE_DIRECTORY}"

Please try it out and tell me if it solves your issue.

@sagiegurari
Copy link
Owner

This is now officially release so i'm closing this issue.

@coffeenotfound
Copy link
Author

This does solve my issue! Thanks for the quick implementation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants