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

Add global config file, implement local and remote registry #817

Merged
merged 148 commits into from
Apr 7, 2023

Conversation

minhqdao
Copy link
Contributor

@minhqdao minhqdao commented Dec 26, 2022

tl;dr

  • Support global config file
  • Support local registry
  • Query and parse remote registry
  • Allow namespace and v (version) specifications for dependencies

Global configuration file

This PR adds a global config file where you can define registry options:

[registry]
path = 'abc' # Location of the local registry (do not use with url or cache_path)
url = 'abc' # Can be used to download packages from a custom remote registry
cache_path = 'abc' # Cache location for downloaded remote packages

The default location of the global config file is ~/.local/share/fpm/config.toml on Unix-based systems and %APPDATA%\local\fpm\config.toml on Windows.

Packages will be queried from the official registry if no config file exists or no [registry] subtable or url were specified.

The default cache_path will be set to ~/.local/share/fpm/dependencies on Unix-based systems and %APPDATA%\local\fpm\dependencies on Windows if not specified.

Both absolute and relative paths can be resolved.

Local Registry

Our goal is to have an official registry that acts as a reliable source for your dependencies. If you somehow prefer to store your dependencies locally, e.g. during development or to have your dependencies bundled with your code, you can define a directory that acts as your local registry in the global config file (#321). fpm will then be able to search the local registry for dependencies listed in the manifest that are not git or direct path dependencies. It can search for the newest version as well as find a specific version.

The local registry needs to be structured in a way that fpm is able to find the correct package. A namespace must be provided and the package must be inside a folder that has the version number as its name. The manifest of an example package called "example" (namespace: "abc", v: "0.1.2") must be located at <path>/abc/example/0.1.2/fpm.toml.

Remote Registry

If url isn't specified in the global config file, dependencies will the drawn from the official registry. In case you decide to use your own custom registry, follow these docs to build your api endpoints. You can query the latest package (according to semantic versioning) or request a specific version. Packages, which are found, will be placed in the cache and are available across the system.

Manifest Changes

namespace and v (version) are added as new keys to manifest dependencies. If you want to use the registry (either local and remote), a namespace must be provided for every package. It will allow using different packages that carry the same name. If you specify v, then fpm will look for that particular version. Without v, you will get the latest version.

Follow-ups:

  • Support entering of a custom path to the global config file via the CLI.
  • Support entering of a path or url to the registry via the CLI.
  • Cache git dependencies as well.
  • Handle constraints.
  • Clear registry cache with fpm clean --registry-cache.
  • Support packages in the cache that have the same version.
  • Document.
  • Implement fpm publish.

Relevant issues: #551, #814.

@awvwgk @everythingfunctional @gnikit @milancurcic @perazz @henilp105 @arteevraina

Copy link
Member

@everythingfunctional everythingfunctional left a comment

Choose a reason for hiding this comment

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

I think this is probably headed in the right direction. Just had a few comments/suggestions.

test/fpm_test/test_registry.f90 Outdated Show resolved Hide resolved
src/fpm_filesystem.F90 Outdated Show resolved Hide resolved
src/fpm_registry.f90 Outdated Show resolved Hide resolved
@minhqdao
Copy link
Contributor Author

Maybe we can get one or two more opinions on this to get it merged soon. I kind of need it to continue working on the local registry. Also this is just the first step of a bigger feature so things might (and most probably will) change on the way. @fortran-lang/fpm

@minhqdao
Copy link
Contributor Author

Will put this on draft, change it up and keep working on it a bit. 🙃

@minhqdao minhqdao marked this pull request as draft December 30, 2022 22:14
@minhqdao minhqdao requested review from awvwgk and everythingfunctional and removed request for awvwgk March 10, 2023 00:28
@minhqdao
Copy link
Contributor Author

Ready for review 😅

@minhqdao minhqdao marked this pull request as draft March 16, 2023 18:25
@minhqdao
Copy link
Contributor Author

Ready for review @henilp105 @arteevraina @perazz

Copy link
Contributor

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Mostly stylistic comments but LGTM, thank you @minhqdao!

app/main.f90 Outdated Show resolved Hide resolved
src/fpm.f90 Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, stylistic changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see what you are referring to.

type downloader_t
contains
procedure, nopass :: get_pkg_data, get_file, unpack
end type
Copy link
Contributor

Choose a reason for hiding this comment

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

For style consistency with fpm, i'ts a good idea to wrap all end all commands with their names (end type downloader_t, end subroutine get_pkg_data, etc...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, of course. But I have to say that I really dislike it because it is nothing but duplicated code. It takes longer to write and refactor. I'd like to see what other reviewers have to say before I change all my end commands. It's not that we've otherwise been very consistent, e.g. in terms of the formatting.

src/fpm/downloader.f90 Outdated Show resolved Hide resolved
json = ptr
end

subroutine get_file(url, tmp_file, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of renaming this to web_get or something that makes it clear we're downloading something from the web, e.g. download_file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you only look at the procedure name, then yes, you are right. But this is a member procedure, meaning that it will, for example, be used as downloader%get_file(). Therefore I think it will always be clear that this procedure downloads a file from the web.

src/fpm_command_line.f90 Outdated Show resolved Hide resolved
fpm.toml Outdated

[dependencies.M_CLI2]
git = "https://github.com/urbanjost/M_CLI2.git"
rev = "7264878cdb1baff7323cc48596d829ccfe7751b8"

[dependencies.jonquil]
Copy link
Contributor

@perazz perazz Mar 16, 2023

Choose a reason for hiding this comment

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

This project seems less active than JSON-Fortran. Maybe please check with @certik which of the JSON packages from fortran-lang authors is more easily compiled with LFortran

Copy link
Contributor Author

@minhqdao minhqdao Apr 2, 2023

Choose a reason for hiding this comment

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

I first tried json-fortran but it's not compatible with fpm because of the many preprocessor commands. It doesn't work with FPM_BOOTSTRAP, for example.

src/fpm_settings.f90 Show resolved Hide resolved
end if
else
call get_global_settings(global_settings, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this read the settings file from disk every time a new dependency is built? If so, this operation could be repeated hundreds of time, I'd rather save it as a global variable to be loaded when fpm is started, or pass it as a dummy argument to this subroutine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right and this should be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been changed here: 0c59079

@minhqdao minhqdao marked this pull request as ready for review April 2, 2023 10:26
private
public :: fpm_global_settings, get_global_settings, get_registry_settings, official_registry_base_url

character(*), parameter :: official_registry_base_url = 'https://fpm-registry.onrender.com'
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this base url out side of the source code in some environment variable and read if from there ?

Copy link
Contributor Author

@minhqdao minhqdao Apr 6, 2023

Choose a reason for hiding this comment

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

You can specify a custom registry url in the global config file if that is what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the base url might not be final here, but it is also nothing that should be set by the user. I'm also a bit puzzled whether we should leave it like that. @awvwgk

Copy link
Member

@arteevraina arteevraina left a comment

Choose a reason for hiding this comment

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

Changes look fine to me overall. Except the one in which we hardcode the base url to the registry which is subject to change for sure.

@minhqdao
Copy link
Contributor Author

minhqdao commented Apr 7, 2023

Thanks everyone, I'll merge this. Next PRs will be shorter :)

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.

5 participants