Skip to content

problems with building with local paths #327

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

Closed
urbanjost opened this issue Dec 22, 2020 · 7 comments · Fixed by #390
Closed

problems with building with local paths #327

urbanjost opened this issue Dec 22, 2020 · 7 comments · Fixed by #390
Labels
bug Something isn't working easy Difficulty level is easy and good for starting into this project

Comments

@urbanjost
Copy link
Contributor

urbanjost commented Dec 22, 2020

The only local paths that work reliably are links that point to a directory within the package.
If you run the "hello_complex" twice it will fail on the second build because of how canon_path assumes all pathnames are relative to the top of the project, and so does not handle pathnames starting with a relative path correctly, and the cache ends up storing something like ".complex_path" instead of "../complex_path"; and full pathnames are stymied by the project dir prefix being appended to the front as "./" so "/share/fpm/..." becomes "./share/fpm/...".

This is particularly vexing for anyone working with packages off-line or at a site with no WWW packages, which is a significant Fortran user base.

It was relatively easy to fix when you could make the assumption all the platforms are Posix and so you can call realpath(3c), and SOME Fortran compilers actually call realpath(3c) when you do an INQUIRE by name (but there is no requirement in the standard for that, and gfortran does not do that).

So either everything has to be bundled into a single package directory or all dependencies have to be from a git repository,
or for some cases you have to delete the build directory between each build.

Should the solution be to pursue a real canonical name routine that would be useful for stdlib also? (I hear that in some circumstances there is no such thing on some Windows machines,but you can usually get "close enough" on normal MSWIndows boxes) or should the current solution be patched up or should the program only support "internal" package copies? The canon_path routine is pretty easy to change to handle paths outside of the project directory, which gets rid of the bad cache files being generated with corrupted relative pathnames, but a real realpath(3c) would have other uses (it is of course trivial to call realpath(3c) itself on POSIX platforms).

It also raises the question of having as part of the QA a complete build of some complex packages that runs TWICE.

@awvwgk
Copy link
Member

awvwgk commented Dec 22, 2020

Thanks for testing, I haven't considered that the canon_path normalization might cause issues. I agree that we have to improve the path handling.

Directories outside of the current scope should be discouraged, but possible with the current infrastructure, keeping leading ../ entries in canon_path should be possible. Also for absolute paths, either a quick check for the nature of the path (relative or absolute) or a more robust join_path routine which returns an absolute path if you join . and absolute path is required.

@LKedward
Copy link
Member

Thanks for reporting @urbanjost. I'll look into solutions for this

@LKedward LKedward self-assigned this Dec 23, 2020
@LKedward
Copy link
Member

An issue with realpath(3) is that it does not work for non-existent paths which is something we need I think.

Working on a possible fix here using getcwd to generate an absolute canonical path. Not yet sure if this is a good solution to pursue or not.

@LKedward LKedward removed their assignment Jan 26, 2021
@awvwgk awvwgk added bug Something isn't working fpm-fortran labels Feb 3, 2021
@awvwgk awvwgk added the easy Difficulty level is easy and good for starting into this project label Feb 12, 2021
@rachittshah
Copy link

Hello @LKedward , @awvwgk and @urbanjost

I'm a new contributor at Fortran. Since I'm hoping to work on FPM via GSoC,I felt this would be a good starting point.

As per my understanding of the issue,we need to give fortran's compiler the right path,while allowing the hierachy in such a way the the n-th iteration of the code wouldn't get confused about the location of the executables,if I'm not wrong.

I've installed fortran,I'm not well-versed with FPM,but I would love to pick it up via this issue.

Will discuss here/on discourse if I have issues.

@LKedward
Copy link
Member

Hi Rachitt @godslayer201 and welcome! This issue a good place to start and we'd certainly appreciate some work on it.

As @urbanjost has described, the problem is primarily with our canon_path routine which isn't working correctly.
The routine should return the same path representation (the canonical path) for all inputs that resolve to the same location.
e.g. ./a and ./dir/../a both represent the same location and so canon_path should return an identical string for both.

One use of canon_path in fpm is to derive compiled object names (see get_object_name) to avoid clashes from source files with the same name but in different directories.
Unfortunately canon_path is not handling paths that begin with .. correctly and is hence causing the issue described above.

As @urbanjost points out, one solution is to use realpath(3) from the C standard library; however my understanding is that realpath requires that the path exists, whereas in fpm we need to use canon_path on non-existent paths sometimes.

My advice would be to focus on fixing/replacing canon_path such that it behaves as required. More specifically I think we need to fix it such that it retains any parent directory references (..) at the beginning of the input path. A good way to start would be to write a unit test with all the necessary cases,
e.g. check that canon_path('../a') == canon_path('../dir/../a') etc.

I'm happy to help you with this, so feel free to ask me questions.

@awvwgk
Copy link
Member

awvwgk commented Mar 17, 2021

Since this issue is blocking #390 I created a hacky fix based on the version string tokenizer. I guess this should cover all cases where the current implementation is failing but we still want a cleaner replacement. A tokenizer with a stack to store the file path seems like the way to go here.

@urbanjost
Copy link
Contributor Author

Might want to throw in #383 and #386 too as the routine is basically replacing the changes for selecting compiler options so making changes to the current master would be lost so those cannot really proceed(?).

@LKedward LKedward linked a pull request Mar 18, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working easy Difficulty level is easy and good for starting into this project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants