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

Combining meta2json.py and rules.py into dromedary.py #2

Closed
Release-Candidate opened this issue Oct 6, 2023 · 8 comments
Closed

Combining meta2json.py and rules.py into dromedary.py #2

Release-Candidate opened this issue Oct 6, 2023 · 8 comments

Comments

@Release-Candidate
Copy link
Contributor

Hi,

I've just combined these two scripts into a single one and before continuing and adding the generation of a opam switch I do have some questions regarding the use of these two scripts:

  • Is the generation of the intermediate JSON output/input necessary for you at Facebook or can I drop it?
  • The standard_library in the generated BUCK file: is this something that is needed by you and I should provide a command line switch for it, or can I always set it to ROOT/lib/ocaml
  • I'd like to a config file which contains info about the Opam switch to create and packages to install in this switch as a first step, before using this newly generated switch as the current one. Is that ok for you? I'd make that part optional, only if such a config file is given as an argument to the script

See #1

@vsiles
Copy link

vsiles commented Oct 9, 2023

I'll have a look at the PR, thank you !
In the meantime:

  • no, the json is not necessary, but it made designing and testing the scripts more easy. Not having to process the Opam switch all the time was quite nice. Also, as you mention, it could be improved to allow "sharing" of config to make more reproducible targets
  • I'll leave @shayne-fletcher give more details, but it is something we'd like to keep as it is
  • As long as you document the feature, I'm ok ;)

facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Oct 9, 2023
Summary: i convinced myself that creating a link to the ocaml standard library directory as extracted from `ocamlopt.opt -config` was necessary and that we can't just assume `$OPAM_SWITCH/lib/ocaml` but have failed to prove that to myself today. together with ocaml-scripts issue [#2](facebook/ocaml-scripts#2) and similar past questions from vsiles i'm motivated to attempt to remove it so that we create but one link into .opam.

Differential Revision: D50082445
facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Oct 9, 2023
Summary:

i convinced myself that creating a link to the ocaml standard library directory as extracted from `ocamlopt.opt -config` was necessary and that we can't just assume `$OPAM_SWITCH/lib/ocaml` but have failed to prove that to myself today. together with ocaml-scripts issue [#2](facebook/ocaml-scripts#2) and similar past questions from vsiles i'm motivated to attempt to remove it so that we create but one link into .opam.

Differential Revision: D50082445
shayne-fletcher pushed a commit to facebook/buck2 that referenced this issue Oct 9, 2023
Summary:

i convinced myself that creating a link to the ocaml standard library directory as extracted from `ocamlopt.opt -config` was necessary and that we can't just assume `$OPAM_SWITCH/lib/ocaml` but have failed to prove that to myself today. together with ocaml-scripts issue [#2](facebook/ocaml-scripts#2) and similar past questions from vsiles i'm motivated to attempt to remove it so that we create but one link into .opam.

Differential Revision: D50082445
@shayne-fletcher
Copy link
Contributor

The standard_library in the generated BUCK file: is this something that is needed by you and I should provide a command line switch for it, or can I always set it to ROOT/lib/ocaml

  • I'll leave @shayne-fletcher give more details, but it is something we'd like to keep as it is

it appears we can always set it to ROOT/lib/ocaml sas suggested by @Release-Candidate.

i have pushed a change internally corresponding to facebook/buck2#442 that will soon make it's way into the facebook/buck2 repo that eliminates the standard_library symlink.

facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Oct 9, 2023
Summary:
Pull Request resolved: #442

i convinced myself that creating a link to the ocaml standard library directory as extracted from `ocamlopt.opt -config` was necessary and that we can't just assume `$OPAM_SWITCH/lib/ocaml` but have failed to prove that to myself today. together with ocaml-scripts issue [#2](facebook/ocaml-scripts#2) and similar past questions from vsiles i'm motivated to attempt to remove it so that we create but one link into .opam.

Reviewed By: vsiles

Differential Revision: D50082445

fbshipit-source-id: e46f2515c8c436cc5291550f6453f200defe0521
@Release-Candidate
Copy link
Contributor Author

Thanks, great, one command line switch less.

A question: what is this resources supposed to contain as last entry? ROOT/lib/ocaml does not work for me.

 command_alias(
     name = "ocamldebug-exe",
     exe = ":ocamldebug",
     resources = [
         ":ocamlrun",
         ":ocamldebug",
         "_opam/lib/ocaml",
     ],

This is the relevant source line: https://github.com/facebook/ocaml-scripts/blob/main/rules.py#L396

I get an error when trying to use that in the BUCK file, removing the entry solves this error:

error: Error coercing attribute `resources` of `root//third-party:ocamldebug-exe`
      --> third-party/BUCK:38:1
       |
    38 | / command_alias(
    39 | |     name = "ocamldebug-exe",
    40 | |     exe = ":ocamldebug",
    41 | |     resources = [
    42 | |         ":ocamlrun",
    43 | |         ":ocamldebug",
    44 | |         "_opam/lib/ocaml",
    45 | |     ],
    46 | |     visibility = [
    47 | |         "PUBLIC",
    48 | |     ],
    49 | | ) if not host_info().os.is_windows else None
       | |_^
       |
    
    
    Error coercing attribute `resources` of `root//third-party:ocamldebug-exe`
    
    Caused by:
        0: Error coercing attribute `resources` of type `attrs.list(attrs.source(), default=[])`
        1: Error coercing [":ocamlrun", ":ocamldebug", "_opam/lib/ocaml"]
        2: Error coercing "_opam/lib/ocaml"
        3: Couldn't coerce `_opam/lib/ocaml` as a source.
             Error when treated as a target: Invalid absolute target pattern `_opam/lib/ocaml` is not allowed: Expected a `:`, a trailing `/...` or the literal `...`.
             Error when treated as a path: Expected file, but got a directory for path `_opam/lib/ocaml` in package `root//third-party`.

@vsiles
Copy link

vsiles commented Oct 10, 2023

I'm not the expert here, @shayne-fletcher do you remember ?
On our internal tool, we have a relative path path/to/lib/ocaml, relative to the location of the BUCK file. Does removing the _ works ? looks like it should be a relative path too from the location of the BUCK file to ocaml lib directory

@Release-Candidate
Copy link
Contributor Author

Release-Candidate commented Oct 10, 2023

Does removing the _ works ? looks like it should be a relative path too from the location of the BUCK file to ocaml lib directory

The path is ROOT/lib/ocaml and is correct, as I'm using a Opam sandbox in third-party directly without a symbolic link to opam, which also works after removing this line. But what I've just noticed, is that it works with a symlink, but not a directory.

So, if opam is a (sym-) link to ./_opam the BUCK config works. When I use the _opam directory directly, it does not.

Which means that the check for a directory does not treat a symlink to a directory as a directory, but as a file :). You might want to look into this, as that may cause real problems elsewhere. Well, actually not a symlink to a directory, but a relative path to a directory that starts with a symlink?!

@Release-Candidate
Copy link
Contributor Author

That does not matter for dromedary.py, as it generates such a symlink to the Opam switch.

@vsiles
Copy link

vsiles commented Oct 17, 2023

For future reference, see most of the discussion happening in #1
@Release-Candidate thank you for your contribution !

@vsiles vsiles closed this as completed Oct 17, 2023
@Release-Candidate
Copy link
Contributor Author

You're welcome, glad I could help.

I've just started working on the config file to generate an Opam switch and install packages.

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

No branches or pull requests

3 participants