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

Allow fpm to change the working directory #483

Merged
merged 3 commits into from Jun 5, 2021

Conversation

awvwgk
Copy link
Member

@awvwgk awvwgk commented May 27, 2021

This allow to use fpm -C <PATH> to change the working directory and enables the automatic discovery of a package manifest in the parent directories.

Explicitly changing of directories

-C, --directory PATH: Change working directory to PATH before running any command.

After checking out this pull request locally, you should be able to build the example package by:

fpm run -- build -C example_packages/hello_fpm

Automatic discovery of manifest files

With a directory changing mechanism we can also easily implement a mechanism to ”find” the package manifest in a parent directory. For this purpose fpm will retrieve the current working directory and check if a package manifest exists in any of the parent directories. By changing to this directory, all other commands can be used directly without modification.

You should be able to check this behavior with

fpm run -- build -C example_packages/hello_fpm/app

If you copy / install the fpm executable you can also check the automatic discovery without the -C / --directory option. It should work just the same.

Please try to break this feature.


Caveats

This introduces preprocessor usage into fpm, not sure if we are actually able to avoid it without using compiler extensions at this point.

Related #481, #172

@awvwgk awvwgk requested a review from certik May 27, 2021 18:12
@certik
Copy link
Member

certik commented May 27, 2021

I don't think this PR closes #481 (or its duplicate #172). To close it, it should work by running fpm without any -C option.

I think this PR closes #411 and #410. My only question would be whether it the flag should be called -C or rather --directory as suggested in those issues.

bind(C, name="chdir")
#else
bind(C, name="_chdir")
#endif
Copy link
Member

Choose a reason for hiding this comment

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

It's fine for this PR, but down the road we should move all these platform specific ifdefs into a platform.f90 or compatibility.f90 file, and then just use such a file here.

Copy link
Member Author

@awvwgk awvwgk May 27, 2021

Choose a reason for hiding this comment

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

I was tempted to do:

#ifndef _WIN32
character(len=*), parameter :: chdir_symbol = "chdir"
#else
character(len=*), parameter :: chdir_symbol = "_chdir"
#endif
! ...
function chdir(...) bind(C, name=chdir_symbol)

But GFortran didn't like those, unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

I was going to try exactly that to see if it works! Then we could have the chdir_symbol in platform.f90. As it is now, we'll have to move the whole chdir definition into platform.f90, which is fine too.

Copy link
Member Author

@awvwgk awvwgk May 27, 2021

Choose a reason for hiding this comment

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

I used string concatenation in the past with bind(C, name=namespace//"...") so in principle this seems to work. But here I'm hitting:

./src/fpm_os.F90:18:29:

   18 |                 bind(C, name=chdir_symbol)
      |                             1
Error: Parameter ‘chdir_symbol’ at (1) has not been declared or is a variable, which does not reduce to a constant expression

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is not possible because the interface is a separate scoping unit not automatically inheriting the module scope. Using import will also not work here because it would make the parameter available after it has been used. That's a tricky one, maybe worth a thread on discourse?

Copy link
Member Author

@awvwgk awvwgk May 27, 2021

Choose a reason for hiding this comment

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

Posted here: https://fortran-lang.discourse.group/t/constant-expressions-for-bind-c-name/1319

Let's see if somebody more knowledgeable in the Fortran standard can help us out here.

@LKedward
Copy link
Member

LKedward commented May 27, 2021

Yes, this would only close #411. The idea for #410 is subtly different and would actually require a different command line option so that it can be used in combination with #411.

Edit: Milan gives a good example and explanation in #411 for how they could be used in combination

@awvwgk
Copy link
Member Author

awvwgk commented May 27, 2021

Okay, than let's try this:

  1. fpm can change with -C or --directory the working directory.
  2. fpm will search within the file system boundaries for an fpm.toml file, if it finds a package manifest, it will change to this directory

Give it a try:

❯ fpm run -- build -C example_packages/hello_fpm/app
fpm: Entering directory '/home/awvwgk/projects/src/git/fortran-package-manager/example_packages/hello_fpm/app'
fpm: Entering directory '/home/awvwgk/projects/src/git/fortran-package-manager/example_packages/hello_fpm'
fpm: Leaving directory '/home/awvwgk/projects/src/git/fortran-package-manager/example_packages/hello_fpm'
fpm: Leaving directory '/home/awvwgk/projects/src/git/fortran-package-manager/example_packages/hello_fpm/app'

@awvwgk awvwgk requested a review from LKedward May 27, 2021 19:55
@LKedward
Copy link
Member

Autodiscovery seems to works nicely but would it not be better to change back to the original directory after the model has been built? This would allow me to do fpm run or fpm test in a different directory to the directory containing the manifest (like #410). By example, if I run git status in a subdirectory, then it finds the git repo in the parent directory but still executes the subcommand as if it's in the current directory.

It seems that we still need two separate options:

What do you think?

@awvwgk
Copy link
Member Author

awvwgk commented May 30, 2021

I think we will need both, knowledge about the working directory and the project root at every point in fpm.

With this patch right now a command like

fpm -C example_packages/hello_fpm run --runner cp -- .

will copy the executable into example_packages/hello_fpm rather than the actual current working directory. Changing this in fpm requires some deeper modification how we handle file paths and how we spawn subprocesses, but this is definitely a desirable change.

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Thanks Sebastian @awvwgk. Changes look good to me for addressing #411 and #172, and I haven't been able to break it. I'm not too worried about the specific syntax in the fpm_os.F90 since this design decision is more suited to discussion in the stdlib_os repo.

ci/run_tests.sh Outdated Show resolved Hide resolved
Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

It looks good to me now. Thanks!

@LKedward
Copy link
Member

LKedward commented Jun 5, 2021

This looks good to go so with two approvals I'll now merge. Thanks @awvwgk

@LKedward LKedward merged commit 845217f into fortran-lang:master Jun 5, 2021
@awvwgk awvwgk deleted the working-directory branch September 25, 2021 17:26
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

3 participants