Added target_os cfg feature to header translator #433

simlay · 2023-04-02T20:56:25Z

I was looking at #408 and this seemed like a nice small unit of work to do on this. I'm not sure what to do with the ios_app_extension and macos_app_extension booleans in the Unavailable struct. To disallow the maccatalyst targets, the nice way to do it is with the not yet stabilized target_abi feature.

simlay · 2023-04-02T21:29:46Z

Hmm. Looks like the submodule diff in CI is failing as this generates a new things.

madsmtm · 2023-04-03T20:26:58Z

Hmm. Looks like the submodule diff in CI is failing as this generates a new things.

Yeah, that's intentional - you should be able to fork the icrate-generated repo and push a commit with the changes there, but if that's troublesome (e.g. if you don't have the correct SDK version) I can do it for you?

simlay · 2023-04-03T21:07:34Z

Well, I updated the submodule ref to a my branch and it's obviously not there. I guess the better way to do this is to merge madsmtm/objc2-generated#6 and then update the ref.

madsmtm · 2023-04-04T05:04:24Z

The result looks nice so far, two things though:

Could we perhaps use the data from Config.libraries to not emit the attributes when not needed (e.g. AppKit is littered with redundant #[cfg(not(any(target_os = "ios")))]).
The reason the CI is failing is because the attribute isn't added in generated/[Framework]/mod.rs yet.

Well, I updated the submodule ref to a my branch and it's obviously not there

I think it worked? Also, you don't need to submit PRs to icrate-generated, I can find them without ;) (and I won't be doing a merge anyhow, just a fast-forward, to keep the size of the repo as minimal as possible while preserving the refs).

maccatalyst

We'll probably need a build script for that in the meantime like we do for objc-sys. I'm fine with it not happening in this PR though.

Sorry for the hazzle btw, I haven't gotten around to writing better docs for how to contribute to header-translator, so it's a bit confusing atm.

simlay · 2023-04-04T19:05:24Z

maccatalyst

We'll probably need a build script for that in the meantime like we do for objc-sys. I'm fine with it not happening in this PR though.

Ah yes. rust-lang/rust#96901 is the not yet stabilized feature for future reference.

madsmtm

Looked it through, it's really good so far, and the output is definitely an improvement!

A few of these are nits, but since they don't interfere with the actual functionality, including the doc_auto_cfg feature (try it with +nightly --features=unstable-docsrs, they're not that important.

Also, I think you have a newer Xcode version than the one icrate is currently built with (which is Xcode 14.2 as pr. the README), which is why the build is failing. I'll try to find some time to update it this week, or perhaps you can do it? (in a separate PR, for the CI part change the DEVELOPER_DIR env variable)

I realize I may be asking a lot here, if you don't feel like doing some of the work I've outlined, it's perfectly fine, your contribution is still very valuable as-is!

crates/header-translator/src/stmt.rs

crates/header-translator/src/availability.rs

madsmtm · 2023-04-07T19:37:19Z

crates/header-translator/src/availability.rs

+            .collect::<Vec<String>>()
+            .join(",");
+        if !unavailable_oses.is_empty() {
+            write!(f, "#[cfg(not(any({unavailable_oses})))]")?;


I know that the headers expose this as "unavailability", but I think it would look nicer in the docs if it was inverted; e.g. that #[cfg(not(any(target_os = "watchos")))] became #[cfg(any(target_os = "tvos", target_os = "ios", target_os = "macos"))] instead (this is also how Apple's own docs does it).

This interferes with GNUStep of course, but perhaps we can just code in a manual override there for all of AppKit and Foundation for now?

I actually spent a little bit of time doing both variants - all(not(target_os = "ios"), any(taget_os = "macos", target_os = "tvos", target_os = "watchos")). I didn't like it as then everything had a cfg attribute.

Yeah, I agree with that part

madsmtm · 2023-04-07T19:49:07Z

crates/header-translator/src/availability.rs

@@ -29,20 +102,27 @@ struct Versions {

 #[derive(Debug, Clone, PartialEq)]
 pub struct Availability {


#[deprecated] now also appears on impl ClassType, which is incorrect.

So perhaps we should split out the Unavailable from Availability (and maybe rename it to PlatformCfg)? Since that part is always going to be a cfg-gate, which has interactions with higher-level cfg-gates (and must propagate to imports like generated/[Framework]/mod.rs), while introduced and deprecated will always only need to apply to the statement/method/field they're associated with.

Maybe it would even make sense to add PlatformCfg as a field of ItemIdentifier? Since these two are access so often together.

crates/header-translator/src/library.rs

madsmtm · 2023-04-07T20:22:15Z

crates/header-translator/src/main.rs

@@ -269,7 +269,7 @@ fn parse_sdk(index: &Index<'_>, sdk: &SdkPath, llvm_target: &str, config: &Confi
                        preprocessing = false;
                        // No more includes / macro expansions after this line
                        let file = library.files.get_mut(&file_name).expect("file");
-                        for stmt in Stmt::parse(&entity, &context) {
+                        for stmt in Stmt::parse(&entity, &context, &library.unavailability) {


Instead of passing the (un-)availability around everywhere like this, I think it would be fine to introduce a few optional fields in Context<'_> that carried this information (since the context is already passed everywhere).

This would also be useful for other things in the future.

So something like (you can freely add only the stuff you need now, just expanding on the example to show the idea):

struct Context<'a> { // ... Existing fields /// Set whenever we know what the current library is. current_library: Cell<Option<Arc<Library>>>, /// Set when parsing methods, struct fields, enum fields, ... current_type: Cell<Option<(ItemIdentifier, Unavailability)>>, } impl Context<'_> { fn library(&self) -> Arc<Library> { self.current_library.clone().take() } fn in_library<R>(&self, library: Arc<Library>, f: impl FnOnce(&Self) -> R) -> R { self.current_library.set(Some(library)); let result = f(self); self.current_library.set(None); result } // ... Similar methods for the others }

I did part of this by adding current_unavailability to Context. In this specific case library is a &mut Library and so passing it in as even a Arc<Library> would require a clone (I think?). Library doesn't derive(Clone)and I didn't feel like adding that. It's definitely cleaner but I'm not in love with it.

madsmtm · 2023-04-07T20:57:22Z

crates/header-translator/src/stmt.rs

+                let unavailable = availability.unavailable.clone();
+                write!(f, "{unavailable}")?;


This should go inside the extern_static! call, and the macro itself in icrate/src/macros.rs should instead be updated to allow it - otherwise, the cfg attribute will be improperly picked up by auto_doc_cfg (it currently works though because generated/[Framework]/mod.rs re-applies the attribute).

Same goes for typed_enum! and typed_extensible_enum!.

Actually, reflecting on this a bit more, perhaps we should instead aim to move as many cfg attributes as we can to outside macro invocations, to allow the compiler to skip evaluating the macro at all?

Actually, reflecting on this a bit more, perhaps we should instead aim to move as many cfg attributes as we can to outside macro invocations, to allow the compiler to skip evaluating the macro at all?

Yeah, I went back and forth on where to put the cfg attributes and felt that outside the macro was better. I should make sure it's consistent though.

For some unclear reason, after updating macOS, I ended up with errors on the enum macros so I put the cfg attributes on all the macros (I think). I had to use just the unavailable part as the #[deprecated] stuff before the macro causes clippy lint issues. I might try and re-add them.

madsmtm · 2023-04-07T21:08:19Z

crates/header-translator/translation-config.toml

+# The MPMediaQueryAddition definition does not have API_UNAVAILABLE annotations
+# where as the MPMediaItem does. This results in problematic conditional compliation:
+# https://github.com/phracker/MacOSX-SDKs/blob/041600eda65c6a668f66cb7d56b7d1da3e8bcc93/MacOSX11.3.sdk/System/Library/Frameworks/MediaPlayer.framework/Versions/A/Headers/MPMediaQuery.h#L103-L118
+[class.MPMediaItem.categories.MPMediaQueryAdditions]


I suspect this is not the only case (it's just the only one caught since we're compiling for macOS), perhaps it would make sense to change Stmt::Methods::category to Option<(ItemIdentifier<Option<String>>, Availability)>?

This is also an argument for adding Unavailable/PlatformCfg to ItemIdentifier, then the problem would neatly go away.

…lity-to-target_os

simlay · 2023-05-21T20:51:12Z

crates/header-translator/src/stmt.rs

@@ -579,7 +579,7 @@ impl Stmt {
                                cls: id.clone(),
                                generics: generics.clone(),
                                category: ItemIdentifier::with_name(None, entity, context),
-                                availability: Availability::parse(entity, context),
+                                availability: availability.clone(),


I don't know that this is the correct way to deal with this. I noticed the generated cfg attributes were gone for superclass functions. I'm guessing that the Availability::parse(entity, context) here checked the availability of the super not the subclass.

marysaka · 2023-07-16T20:36:54Z

Any news about that PR? What is missing for this to be ready?

I'm quite interested to get this working for some projects.

simlay · 2023-07-16T20:42:43Z

Any news about that PR? What is missing for this to be ready?

Hmm. Looks like I need to deal with some merge latest conflicts. I think the thing that was most lacking was generated bindings used xcode 14.2 (latest stable) but CI uses an older version.

@madsmtm What do you think?

I'm quite interested to get this working for some projects.

Glad to hear it!

simlay added 3 commits April 2, 2023 16:45

Added target_os cfg feature to header translator

fabf5ed

Removed irrelevant comment

7de510b

Remove empty new lines from generation

615c4de

simlay mentioned this pull request Apr 3, 2023

Updated generations for target_os cfg feature madsmtm/objc2-generated#6

Closed

Update ref to submodule

429ac57

simlay added 12 commits April 4, 2023 15:08

Added library_unavailability to remove un-needed config attributes

3db4f6c

Removed cfg target for maccatalyst

57d3bd7

Made use of cfg target in library imports

39ce7d5

cargo fmt

e04eaa8

submodule bump

cce841a

update submodule

747216d

Fix compiling for macos

ae84ab9

Fix iOS and macOS compiling

5fb969e

Fixed enum variant imports

f9d3d4f

cargo fmt

492e0a8

Fix clippy lints maybe

7edae2c

Fix clippy lint

61d1aa7

madsmtm requested changes Apr 7, 2023

View reviewed changes

simlay added 7 commits April 9, 2023 16:21

Updates from code review

13a8a32

Remove any when theres only one os

4b34fd4

Move library_unavailability into context

677ae87

cargo clippy

0df1d04

Merge branch 'master' of github.com:madsmtm/objc2 into add-unavailabi…

fb2265a

…lity-to-target_os

cargo fmt

9bd8ab3

Update submodule for new header-translator

dbe1782

Fix superclass availability

f1791dc

simlay commented May 21, 2023

View reviewed changes

madsmtm mentioned this pull request May 21, 2024

Emit target_os = ... cfgs on framework dependencies #618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added target_os cfg feature to header translator #433

Added target_os cfg feature to header translator #433

simlay commented Apr 2, 2023

simlay commented Apr 2, 2023

madsmtm commented Apr 3, 2023 •

edited

simlay commented Apr 3, 2023

madsmtm commented Apr 4, 2023 •

edited

simlay commented Apr 4, 2023

madsmtm left a comment

madsmtm Apr 7, 2023 •

edited

simlay Apr 9, 2023

madsmtm Apr 13, 2023

madsmtm Apr 7, 2023

madsmtm Apr 7, 2023

simlay May 9, 2023

madsmtm Apr 7, 2023

madsmtm Apr 7, 2023

simlay Apr 8, 2023

simlay Apr 9, 2023

madsmtm Apr 7, 2023

simlay May 21, 2023

marysaka commented Jul 16, 2023

simlay commented Jul 16, 2023

		@@ -29,20 +102,27 @@ struct Versions {

		#[derive(Debug, Clone, PartialEq)]
		pub struct Availability {

		let unavailable = availability.unavailable.clone();
		write!(f, "{unavailable}")?;

Added target_os cfg feature to header translator #433

Are you sure you want to change the base?

Added target_os cfg feature to header translator #433

Conversation

simlay commented Apr 2, 2023

simlay commented Apr 2, 2023

madsmtm commented Apr 3, 2023 • edited

simlay commented Apr 3, 2023

madsmtm commented Apr 4, 2023 • edited

simlay commented Apr 4, 2023

madsmtm left a comment

Choose a reason for hiding this comment

madsmtm Apr 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marysaka commented Jul 16, 2023

simlay commented Jul 16, 2023

madsmtm commented Apr 3, 2023 •

edited

madsmtm commented Apr 4, 2023 •

edited

madsmtm Apr 7, 2023 •

edited