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

Fix: resolve our Clojure source dependencies first in the classpath #17919

Merged
merged 2 commits into from
Nov 18, 2023

Conversation

ilmotta
Copy link
Contributor

@ilmotta ilmotta commented Nov 15, 2023

Summary

In the (currently open) malli PR #17867 (comment), @pavloburykh reported the PR build wasn't working and all e2e failed.

The root of the problem is that in our malli PR we have a namespace named schema.core, but this namespace is taken by library prismatic/schema already (see source), a library used by our direct dependency onbidi 2.1.6. This leads to a broken build where the ClojureScript compiler reports undeclared vars (https://clojurescript.org/reference/compiler-options#warnings).

This PR changes the order Java resolves dependencies via the classpath mechanism. We now first resolve our own Clojure sources, and then project dependencies.

Note

I tested this solution is correct in this WIP commit bd5b69b, which I extracted for this new PR. Once this PR is merged, I'll remove the WIP commit from the malli PR and rebase from develop.

Platforms

  • Android
  • iOS

status: ready

@ilmotta ilmotta added the nix label Nov 15, 2023
@ilmotta ilmotta self-assigned this Nov 15, 2023
@ilmotta ilmotta requested a review from jakubgs as a code owner November 15, 2023 19:13
@status-im-auto
Copy link
Member

status-im-auto commented Nov 15, 2023

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4d793bc #1 2023-11-15 19:24:29 ~10 min android-e2e 🤖apk 📲
✔️ 4d793bc #1 2023-11-15 19:24:29 ~10 min android 🤖apk 📲
✔️ 4d793bc #1 2023-11-15 19:24:33 ~10 min tests 📄log
✔️ 4d793bc #1 2023-11-15 19:25:00 ~11 min ios 📱ipa 📲
d81831b #2 2023-11-15 19:32:41 ~2 min tests 📄log
✔️ d81831b #2 2023-11-15 19:36:48 ~6 min android-e2e 🤖apk 📲
✔️ d81831b #2 2023-11-15 19:37:09 ~6 min android 🤖apk 📲
✔️ d81831b #2 2023-11-15 19:41:13 ~10 min ios 📱ipa 📲
✔️ afe7572 #3 2023-11-15 20:53:48 ~5 min ios 📱ipa 📲
✔️ afe7572 #3 2023-11-15 20:54:38 ~6 min android-e2e 🤖apk 📲
✔️ afe7572 #3 2023-11-15 20:57:32 ~8 min android 🤖apk 📲
✔️ afe7572 #3 2023-11-15 20:57:51 ~9 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 629a5ef #4 2023-11-15 23:08:20 ~6 min android-e2e 🤖apk 📲
✔️ 629a5ef #4 2023-11-15 23:09:40 ~7 min android 🤖apk 📲
✔️ 629a5ef #4 2023-11-15 23:12:04 ~10 min tests 📄log
✔️ 629a5ef #4 2023-11-15 23:14:40 ~12 min ios 📱ipa 📲
✔️ c9c74e8 #5 2023-11-18 10:49:48 ~9 min android-e2e 🤖apk 📲
✔️ c9c74e8 #5 2023-11-18 10:50:02 ~10 min android 🤖apk 📲
✔️ c9c74e8 #5 2023-11-18 10:50:37 ~10 min tests 📄log
✔️ c9c74e8 #5 2023-11-18 10:51:49 ~11 min ios 📱ipa 📲

@ilmotta
Copy link
Contributor Author

ilmotta commented Nov 15, 2023

Weirdly, the CI failed make lint due to:

[2023-11-15T19:31:57.107Z] /tmp/tmp-status-mobile-jenkins-status-mobile-prs-tests-PR-17919-2/nix-shell-3212512-0/rc: line 10: node_modules/.bin/prettier: No such file or directory

But locally it's fine.

@ilmotta ilmotta force-pushed the change-classpath-source-order branch 2 times, most recently from afe7572 to 629a5ef Compare November 15, 2023 23:01
@yqrashawn
Copy link
Contributor

Does this mean we overwrite schema.core in deps with schema.core in src?

bidi is used for universal link and I don't think we are using any of its features that rely on prismatic/schema
If prismatic/schema got overwrote, it might cause problems once we use those features or introduce new libraries that use prismatic/schema

Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

A very good catch.

@ilmotta
Copy link
Contributor Author

ilmotta commented Nov 16, 2023

bidi is used for universal link and I don't think we are using any of its features that rely on prismatic/schema If prismatic/schema got overwrote, it might cause problems once we use those features or introduce new libraries that use prismatic/schema

That's the confusing part that took me a long time to figure out @yqrashawn. We have many indirect dependencies, such as prismatic/schema (used by bidi). These indirect dependencies are also put at the front of the classpath resolution, before status-mobile/src dependencies are resolved.

This problem does not exist if we call shadow-cljs release mobile, like most shadow-cljs projects would release. It's only via our Nix layer that the problem exists due to the manipulation of the classpath.

Even if we stop using bidi (and thus our indirect dependency on prismatic/schema) I think this PR is still a step in the right direction.

Does this mean we overwrite schema.core in deps with schema.core in src?

Yes, it means our src/* namespaces will be resolved before our direct and indirect dependencies. This problem is probably very rare in most Clojure projects because in those projects they always have a root namespace with the name of the product, which is hopefully unique. In status-mobile we don't do this to make some namespaces shorter.

Copy link
Contributor

@yqrashawn yqrashawn left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, LGTM

@ilmotta ilmotta force-pushed the change-classpath-source-order branch from 629a5ef to c9c74e8 Compare November 18, 2023 10:39
@status-im-auto
Copy link
Member

89% of end-end tests have passed

Total executed tests: 45
Failed tests: 1
Expected to fail tests: 4
Passed tests: 40
IDs of failed tests: 702777 
IDs of expected to fail tests: 702732,703503,702731,702808 

Failed tests (1)

Click to expand
  • Rerun failed tests

  • Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_add_contact_field_validation, id: 702777

    Device 2: Tap on found: Button
    Device 2: Wait for element `Button` for max 30s and click when it is available

    activity_center/test_activity_center.py:145: in test_add_contact_field_validation
        self.device_2.create_user(username=new_username_2, first_user=False)
    ../views/sign_in_view.py:249: in create_user
        self.identifiers_button.wait_and_click(30)
    ../views/base_element.py:96: in wait_and_click
        self.wait_for_visibility_of_element(sec)
    ../views/base_element.py:139: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: Button by accessibility id:`skip-identifiers` is not found on the screen after wait_for_visibility_of_element
    



    Device sessions

    Expected to fail tests (4)

    Click to expand

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    2. test_group_chat_offline_pn, id: 702808

    Device 3: Looking for a message by text: message from old member
    Device 3: Looking for a message by text: message from new member

    critical/chats/test_group_chat.py:323: in test_group_chat_offline_pn
        self.errors.verify_no_errors()
    base_test_case.py:191: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Messages PN was not fetched from offline 
    

    [[Data delivery issue]]

    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_pin_messages, id: 702731

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Pin feature is in development]]

    Class TestCommunityOneDeviceMerged:

    1. test_community_discovery, id: 703503

    Test is not run, e2e blocker  
    

    [[reason: [NOTRUN] Curated communities not loading, https://github.com//issues/17852]]

    Passed tests (40)

    Click to expand

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_mute_chat, id: 703495
    Device sessions

    2. test_group_chat_send_image_save_and_share, id: 703297
    Device sessions

    3. test_group_chat_reactions, id: 703202
    Device sessions

    4. test_group_chat_join_send_text_messages_push, id: 702807
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_several_images_send_reply, id: 703194
    Device sessions

    2. test_community_one_image_send_reply, id: 702859
    Device sessions

    3. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    4. test_community_mark_all_messages_as_read, id: 703086
    Device sessions

    5. test_community_contact_block_unblock_offline, id: 702894
    Device sessions

    6. test_community_message_delete, id: 702839
    Device sessions

    7. test_community_message_send_check_timestamps_sender_username, id: 702838
    Device sessions

    8. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    9. test_community_message_edit, id: 702843
    Device sessions

    10. test_community_unread_messages_badge, id: 702841
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

    1. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    2. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
    Device sessions

    3. test_1_1_chat_mute_chat, id: 703496
    Device sessions

    Class TestActivityCenterContactRequestMultipleDevicePR:

    1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
    Device sessions

    2. test_activity_center_contact_request_decline, id: 702850
    Device sessions

    Class TestActivityMultipleDevicePRTwo:

    1. test_activity_center_mentions, id: 702957
    Device sessions

    2. test_activity_center_admin_notification_accept_swipe, id: 702958
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
    Device sessions

    2. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    3. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    4. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    5. test_1_1_chat_edit_message, id: 702855
    Device sessions

    6. test_1_1_chat_send_image_save_and_share, id: 703391
    Device sessions

    7. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    2. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    3. test_community_undo_delete_message, id: 702869
    Device sessions

    4. test_community_navigate_to_channel_when_relaunch, id: 702846
    Device sessions

    5. test_community_mute_community_and_channel, id: 703382
    Device sessions

    Class TestCommunityMultipleDeviceMergedTwo:

    1. test_community_markdown_support, id: 702809
    Device sessions

    2. test_community_hashtag_links_to_community_channels, id: 702948
    Device sessions

    3. test_community_mentions_push_notification, id: 702786
    Device sessions

    4. test_community_leave, id: 702845
    Device sessions

    5. test_community_join_when_node_owner_offline, id: 703629
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_navigation_jump_to, id: 702936
    Device sessions

    2. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    @ilmotta ilmotta merged commit 17ebedd into develop Nov 18, 2023
    @ilmotta ilmotta deleted the change-classpath-source-order branch November 18, 2023 11:50
    yevh-berdnyk pushed a commit that referenced this pull request Dec 8, 2023
    …17919)
    
    In PR #17867 we have a namespace
    named schema.core, but this namespace is taken by library prismatic/schema
    already (see https://github.com/plumatic/schema/tree/master/src/cljc/schema), a
    library used by our direct dependency on bidi 2.1.6. This leads to a broken
    build where the ClojureScript compiler reports undeclared vars
    (https://clojurescript.org/reference/compiler-options#warnings).
    
    We change the order Java resolves dependencies via the classpath mechanism. We
    now first resolve our own Clojure sources, and then project dependencies.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    Archived in project
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    6 participants