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.

Sign up for GitHub

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

Jump to bottom

exe not recognized as a format #194

Closed
SolidTux opened this issue Dec 19, 2024 · 6 comments · Fixed by #196 or #197
Closed

exe not recognized as a format #194

SolidTux opened this issue Dec 19, 2024 · 6 comments · Fixed by #196 or #197

Comments

@SolidTux
Copy link

Despite the help listing exe as a valid value for format, it is not accepted.

$ x build --platform windows --arch x64 --format exe
error: invalid value 'exe' for '--format <FORMAT>': unsupported arch exe

Also note that it wrongly says, "unsupported arch" instead of format.

But, when not specifying any format, an exe is built, so the process itself seems to be working.
@MarijnS95
Copy link
Member

MarijnS95 commented Dec 19, 2024 •
edited
Loading

Looks like a copy-paste error when xbuild was originally made EDIT: it was missed when the Exe variant was added later in da16695, "exe" is not in the match:

xbuild/xbuild/src/lib.rs

Lines 170 to 182 in ffe3e34

fn from_str(arch: &str) -> Result<Self> {
Ok(match arch {
"aab" => Self::Aab,
"apk" => Self::Apk,
"appbundle" => Self::Appbundle,
"appdir" => Self::Appdir,
"appimage" => Self::Appimage,
"dmg" => Self::Dmg,
"ipa" => Self::Ipa,
"msix" => Self::Msix,
_ => anyhow::bail!("unsupported arch {}", arch),
})
}

But ::Exe is the default when building for --platform windows:

xbuild/xbuild/src/lib.rs

Lines 196 to 197 in ffe3e34

(Platform::Windows, Opt::Debug) => Self::Exe,
(Platform::Windows, Opt::Release) => Self::Exe, // TODO: Msix

@MarijnS95
Copy link
Member

MarijnS95 commented Dec 19, 2024 •
edited
Loading

Despite the help listing exe as a valid value for format

Also, this is weird. Didn't we autogenerate the CLI (help and string parsers) with clap?

EDIT: Yikes, this is hardcoded instead of autogenerated by the clap crate:

xbuild/xbuild/src/lib.rs

Lines 391 to 395 in ffe3e34

/// Build artifacts with format. Can be one of `aab`,
/// `apk`, `appbundle`, `appdir`, `appimage`, `dmg`,
/// `exe`, `ipa`, `msix`.
#[clap(long, conflicts_with = "store")]
format: Option<Format>,

@SolidTux
Copy link
Author

xbuild/xbuild/src/lib.rs

Lines 170 to 182 in ffe3e34

fn from_str(arch: &str) -> Result<Self> {
Ok(match arch {
"aab" => Self::Aab,
"apk" => Self::Apk,
"appbundle" => Self::Appbundle,
"appdir" => Self::Appdir,
"appimage" => Self::Appimage,
"dmg" => Self::Dmg,
"ipa" => Self::Ipa,
"msix" => Self::Msix,
_ => anyhow::bail!("unsupported arch {}", arch),
})
}

Also, here it's 4 times arch, that should be platform. In general, that should be a rather easy fix, I can try to do it. But I'm not sure if I'll still get around to it this year.

@MarijnS95
Copy link
Member

As usual PRs are welcome to fix any issue, including such little inconsistencies.

@MarijnS95 MarijnS95 mentioned this issue Dec 22, 2024
Merged
MarijnS95 added a commit that referenced this issue Dec 22, 2024
@MarijnS95
Replace broken open-coding of FromStr with derive(clap::ValueEnum)
95a6d58 
We've seen that `impl FromStr for Format` missed the `"exe"` match arm
in parsing (#194) despite being listed in the allowed values in our
top-level documentation and help text.

Instead of fixing this mistake, remove open-coded `FromStr` entirely and
rely on `clap`'s excellent `ValueEnum` derive.  This not only generates
a **complete** parser (with the option for attributes to change variant
names) but also automates generation of a list of possible values,
getting rid of any ambiguity/duplication/copy-paste errors we might have
or create in the future:

    ❯ cargo r -- build -h
    ...
    Usage: x build [OPTIONS]

    Options:
          ...
          --platform <PLATFORM>
              Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
          --arch <ARCH>
              Build artifacts for target arch [possible values: arm64, x64]
          ...
          --format <FORMAT>
              Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
          --store <STORE>
              Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated `PossibleValue::get_name()` to
implement our open-coded `Display` implementations, ensuring any renames
via clap attributes trickle through into how we print them.
@MarijnS95 MarijnS95 mentioned this issue Dec 22, 2024
Merged
MarijnS95 added a commit that referenced this issue Dec 27, 2024
@MarijnS95
Replace broken open-coding of FromStr with derive(clap::ValueEnum)
cd8eb65 
We've seen that `impl FromStr for Format` missed the `"exe"` match arm
in parsing (#194) despite being listed in the allowed values in our
top-level documentation and help text.

Instead of fixing this mistake, remove open-coded `FromStr` entirely and
rely on `clap`'s excellent `ValueEnum` derive.  This not only generates
a **complete** parser (with the option for attributes to change variant
names) but also automates generation of a list of possible values,
getting rid of any ambiguity/duplication/copy-paste errors we might have
or create in the future:

    ❯ cargo r -- build -h
    ...
    Usage: x build [OPTIONS]

    Options:
          ...
          --platform <PLATFORM>
              Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
          --arch <ARCH>
              Build artifacts for target arch [possible values: arm64, x64]
          ...
          --format <FORMAT>
              Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
          --store <STORE>
              Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated `PossibleValue::get_name()` to
implement our open-coded `Display` implementations, ensuring any renames
via clap attributes trickle through into how we print them.
@SolidTux
Copy link
Author

SolidTux commented Jan 2, 2025

I'm a bit confused about the two PRs, which one do you want me to test?

@MarijnS95
Copy link
Member

Let's start with #196, if that works I'll follow up with #197 either.

Don't want to sneak bugfixes in with code improvements.

MarijnS95 added a commit that referenced this issue Jan 3, 2025
@MarijnS95
Replace broken open-coding of FromStr with derive(clap::ValueEnum)
d9da872 
We've seen that `impl FromStr for Format` missed the `"exe"` match arm
in parsing (#194) despite being listed in the allowed values in our
top-level documentation and help text.

Instead of fixing this mistake, remove open-coded `FromStr` entirely and
rely on `clap`'s excellent `ValueEnum` derive.  This not only generates
a **complete** parser (with the option for attributes to change variant
names) but also automates generation of a list of possible values,
getting rid of any ambiguity/duplication/copy-paste errors we might have
or create in the future:

    ❯ cargo r -- build -h
    ...
    Usage: x build [OPTIONS]

    Options:
          ...
          --platform <PLATFORM>
              Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
          --arch <ARCH>
              Build artifacts for target arch [possible values: arm64, x64]
          ...
          --format <FORMAT>
              Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
          --store <STORE>
              Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated `PossibleValue::get_name()` to
implement our open-coded `Display` implementations, ensuring any renames
via clap attributes trickle through into how we print them.
MarijnS95 added a commit that referenced this issue Jan 3, 2025
@MarijnS95
Replace broken open-coding of FromStr with derive(clap::ValueEnum) (
f7c8252 
#197)

We've seen that `impl FromStr for Format` missed the `"exe"` match arm
in parsing (#194) despite being listed in the allowed values in our
top-level documentation and help text.

Instead of fixing this mistake, remove open-coded `FromStr` entirely and
rely on `clap`'s excellent `ValueEnum` derive.  This not only generates
a **complete** parser (with the option for attributes to change variant
names) but also automates generation of a list of possible values,
getting rid of any ambiguity/duplication/copy-paste errors we might have
or create in the future:

    ❯ cargo r -- build -h
    ...
    Usage: x build [OPTIONS]

    Options:
          ...
          --platform <PLATFORM>
              Build artifacts for target platform [possible values: android, ios, linux, macos, windows]
          --arch <ARCH>
              Build artifacts for target arch [possible values: arm64, x64]
          ...
          --format <FORMAT>
              Build artifacts with format [possible values: aab, apk, appbundle, appdir, appimage, dmg, exe, ipa, msix]
          --store <STORE>
              Build artifacts for target app store [possible values: apple, microsoft, play, sideload]

Finally, we also utilize its generated `PossibleValue::get_name()` to
implement our open-coded `Display` implementations, ensuring any renames
via clap attributes trickle through into how we print them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@MarijnS95 @SolidTux