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

How to require an specific OS #106

Open
macintoshplus opened this issue Nov 19, 2024 · 15 comments · May be fixed by #116
Open

How to require an specific OS #106

macintoshplus opened this issue Nov 19, 2024 · 15 comments · May be fixed by #116
Assignees
Labels
enhancement New feature or request

Comments

@macintoshplus
Copy link

The Win32Service PHP extension can be built and used only on the Microsoft Windows operating system.

How to specify this constraint on composer.json file?

@asgrim asgrim added the enhancement New feature or request label Nov 19, 2024
@asgrim
Copy link
Collaborator

asgrim commented Nov 19, 2024

Not possible at the moment, but something we can look into 👍

@alexandre-daubois
Copy link
Contributor

I'd be interested in having a look at this feature! What about the following syntax?

{
    "name": "myvendor/myext",
    "php-ext": {
        "extension-name": "myext",
        "configure-options": [
            // ...
        ],
        "os-families": ['Windows', 'Darwin'] // not compatible with Linux here for example
    }
}

The values contained in os-families would have to match PHP_OS_FAMILY.

Also, it would be possible to use the wildcard, like "os-families": ["*"] (or "os-families": "*"?) if one wants to be explicit on supporting all operating systems.

@Seldaek
Copy link

Seldaek commented Nov 20, 2024

I think it'd be good to have a way to negate something if you want to closely match PHP_OS_FAMILY, because I suspect most things are gonna be Windows-only, Linux/BSD/Solaris/Darwin-only, or everything. I don't think there are many that support everything except Solaris or everything except BSD.

So it could be nicer with ["!Windows"] or something? I'm just thinking it would suck if people list Linux and forget to include BSD/Solaris/Darwin and then things turn out not installable for no good reason.

Another possible syntax would be "os-families-exclude": ["Windows"]

@alexandre-daubois
Copy link
Contributor

I agree with you, by reading at your comment it feels more natural to me to work with a blacklist instead of a whitelist. Also it saves us from handling the wildcard. I'd rather use os-families-exclude instead of the bit-less explicit ! operator indeed.

@asgrim
Copy link
Collaborator

asgrim commented Nov 20, 2024

Some examples for reference:

only Win:

"os-families-exclude": ["BSD", "Darwin", "Solaris", "Linux", "Unknown"]

only non-Win:

"os-families-exclude": ["Windows"]

only OSX:

"os-families-exclude": ["Windows", "BSD", "Solaris", "Linux", "Unknown"]

@Seldaek
Copy link

Seldaek commented Nov 21, 2024

So what I was suggesting is to include BOTH options sorry it was clearly not clear :D

only Win:

"os-families": ["Windows"]

only non-Win:

"os-families-exclude": ["Windows"]

only OSX:

"os-families-exclude": ["Darwin"]

That way you can target whatever you want with the least amount of listing-distros and possibly forgetting things.

Obviuosly the "Unknown" wildcard is a bit problematic here.. But I guess those fall in the category of "rather just allow it and see if it builds 🤷🏻‍♂️ "?

@alexandre-daubois
Copy link
Contributor

alexandre-daubois commented Nov 21, 2024

I'm fine accepting both keys, I agree it's more flexible because I guess you'll either exclude only one OS or allow only one in most cases actually.

About the "Unknown" value, I also think we shouldn't get too worked up about it and we shouldn't do anything special for it. Let's just wait for someone to raise an issue if there's something wrong 😄

@macintoshplus
Copy link
Author

I’m okay to use both keys. The wildcard or “Unknown” is not necessary.

If the extension maintainer wants to exclude the unknown OS, it must list all included OS.

If the extension maintainer wants to include “unknown” it uses the exclude key or nothing.

IMO it’s necessary to make an exhaustive list of OS names used in the lists.

@asgrim
Copy link
Collaborator

asgrim commented Nov 21, 2024

IMO it’s necessary to make an exhaustive list of OS names used in the lists.

This list is already defined: https://www.php.net/manual/en/reserved.constants.php#constant.php-os-family

That list is where Unknown comes from.

One thing that should be defined is the behaviour if both keys are used, a few examples:

{
  "php-ext": {
    "os-families": ["Windows"],
    "os-families-exclude": ["Windows"]
  }
}
{
  "php-ext": {
    "os-families": ["Windows", "Linux"],
    "os-families-exclude": ["Linux"]
  }
}

I would suggest that perhaps the most natural method for processing would be (pseudo-code):

allowedPlatforms = Windows BSD Darwin Solaris Linux Unknown

if "os-families" is specified: replace allowedPlatforms with the list

if "os-families-exclude" is specified: subtract from the allowedPlatforms

Therefore the results of my two examples:

  • first one is not installable anywhere
  • second one is only installable on Windows

@Seldaek
Copy link

Seldaek commented Nov 21, 2024

I think I'd declare it in the json schema as being a oneOf, so that either key should be provided, but not both, and probably need to check in the loader code to throw if both are provided because this is nonsense, even if we can reconcile it per your rules, it's just clearer to disallow it IMO.

@asgrim
Copy link
Collaborator

asgrim commented Nov 21, 2024

That is probably safer 💯 👍

@macintoshplus
Copy link
Author

Both keys are declared nonsense. However, the maintainer must check if the composer.json is valid before tagging the version.

A command validate must be added to help to validate the file.

@alexandre-daubois
Copy link
Contributor

I updated https://github.com/ThePHPF/pie-design/pull/29/files to reflect our discussion.

@macintoshplus the validate command seems a nice idea to me. This may be worth discuss about it in a new issue 🙂

@asgrim
Copy link
Collaborator

asgrim commented Nov 21, 2024

A command validate must be added to help to validate the file.

composer validate already exists which checks the schema, example:

$ composer validate

In Factory.php line 317:
                                                                          
  "./composer.json" does not match the expected JSON schema:              
   - php-ext.extension-name : NULL value found, but a string is required  
   - php-ext.priority : String value found, but an integer is required    
                                                                          

validate [--no-check-all] [--check-lock] [--no-check-lock] [--no-check-publish] [--no-check-version] [-A|--with-dependencies] [--strict] [--] [<file>]

@macintoshplus
Copy link
Author

Cool, less code to write. But, it's interesting to add this information to the documentation. Right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants