Update Makefile and CI scripts to remove 'ci' folder and 'ci-' prefix. #130

kevinlind · 2024-02-05T05:15:25Z

Description

Update Makefile and CI scripts to remove the "ci" folder during builds and "ci-" prefix in Makefile targets. The use of the "ci" folder is no longer needed. Additionally, the copy step to the "ci" folder may get skipped if an earlier step fails and aborts the task. It is less error prone to reference files directly from the build folders than using the "ci" folder.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

kevinlind · 2024-02-05T05:16:05Z

Makefile

 	(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) lint)

-
-ci-build: create-ci


Move below and renamed to "assemble-phone"

kevinlind · 2024-02-05T05:16:21Z

Makefile

-	(mv $(AAR_FILE_DIR)/$(EXTENSION-LIBRARY-FOLDER-NAME)-phone-release.aar  $(AAR_FILE_DIR)/$(MODULE_NAME)-release-$(LIB_VERSION).aar)
-	(cp -r ./code/$(EXTENSION-LIBRARY-FOLDER-NAME)/build $(BUILD-ASSEMBLE-LOCATION))
-
-ci-build-app:


Moved below and renamed to "assemble-phone"

kevinlind · 2024-02-05T05:18:36Z

Makefile

-BUILD-ASSEMBLE-LOCATION = ./ci/assemble
-ROOT_DIR=$(shell git rev-parse --show-toplevel)
-
-PROJECT_NAME = $(shell cat $(ROOT_DIR)/code/gradle.properties | grep "moduleProjectName" | cut -d'=' -f2)
-AAR_NAME = $(shell cat $(ROOT_DIR)/code/gradle.properties | grep "moduleAARName" | cut -d'=' -f2)
-MODULE_NAME = $(shell cat $(ROOT_DIR)/code/gradle.properties | grep "moduleName" | cut -d'=' -f2)
-LIB_VERSION = $(shell cat $(ROOT_DIR)/code/gradle.properties | grep "moduleVersion" | cut -d'=' -f2)
-SOURCE_FILE_DIR =  $(ROOT_DIR)/code/$(PROJECT_NAME)
-AAR_FILE_DIR =  $(ROOT_DIR)/code/$(PROJECT_NAME)/build/outputs/aar


I removed the step from "ci-build"/"assemble-phone" which renamed the assembled .aar file, which required all these variables. I don't think we need to rename the .aar anymore as it is not used in the publish task anymore.

kevinlind · 2024-02-05T05:19:31Z

Makefile

 	(./code/gradlew -p code/${EXTENSION-LIBRARY-FOLDER-NAME} publishReleasePublicationToSonatypeRepository)

-ci-publish-main: clean build-release
+publish-main: clean assemble-phone-release


I've kept this as publish-main but I'm open to just renaming it publish, similar to the User Profile project.

Userprofile keep them as
ci-publish-staging
ci-publish

Should we follow to make it standard?

It seems to be a mistake that they kept the ci- prefix just for those two targets. There are other targets which are called in CI which do not use the prefix.
I'm fine with renaming from publish-main to just publish.

Core team responded saying the ci- prefix is preserved because these targets are meant to only be run in CI. I've updated the Make file to add the ci- prefix back while renaming ci-publish-main to ci-publish.

codecov · 2024-02-05T05:31:28Z

Codecov Report

Merging #130 (920d254) into dev-v3.0.0 (7e293dc) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-v3.0.0     #130   +/-   ##
=============================================
  Coverage         84.30%   84.30%           
  Complexity          406      406           
=============================================
  Files                30       30           
  Lines              1643     1643           
  Branches            235      235           
=============================================
  Hits               1385     1385           
  Misses              165      165           
  Partials             93       93

Flag	Coverage Δ
functional-tests	`67.07% <ø> (ø)`
unit-tests	`80.34% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

cacheung · 2024-02-05T18:42:13Z

.circleci/config.yml

      - store_artifacts:
-          path: ci/javadoc/build/reports
+          path: code/edge/build/dokka/javadoc


Should we put into
code/edge/build/docs/javadoc
as Userprofile?

This is reading the generated HTML files after running Javadoc. Edge is using Dokka to generate the Javadocs so this is the path where the files are located. We may want to move away from Dokka, as it appears the other extensions using Kotlin are not using it. But if we do, that will be in another PR.

cacheung · 2024-02-05T18:44:21Z

.circleci/config.yml

@@ -134,13 +134,12 @@ jobs:
      # Code coverage upload using Codecov
      # See options explanation here: https://docs.codecov.com/docs/codecov-uploader
      - codecov/upload:
-          file: ci/unit-test/build/reports/jacoco/platformUnitTestJacocoReport/platformUnitTestJacocoReport.xml
+          file: code/edge/build/reports/jacoco/platformUnitTestJacocoReport/platformUnitTestJacocoReport.xml


How about use
code/edge/build/reports/coverage/test/phone/debug/report.xml

That directory doesn't exist in Edge when running the unit tests. Keep in mind I'm only renaming the Makefile targets and I haven't changed the Gradle files. If the common Gradle files change how Jacoco is setup, then these file locations may change later.

I've updated the Gradle task used to generate the coverage reports and the generated report location. The report location now matches what you suggested. Thanks!

cacheung · 2024-02-05T18:46:53Z

.circleci/config.yml


      - android/run-tests:
-          test-command: make ci-unit-test
+          test-command: make unit-test


Userprofile uses make unit-test-coverage which include running unit test and create coverage. Do you want to include that in this PR or later?

It's not clear to me that it is needed to have a separate Makefile target which just runs the unit-test without producing a coverage file. I don't see that User Profile is running their unit-test target, only unit-test-coverage. I would prefer to keep the target name as unit-test while having it produce the coverage files.

They have both unit-test and unit-test-coverage.
unit-test can be used for running locally.
unit-test-coverage will run both unit test and unit test coverage.

I've updated the Make file to include coverage and non-coverage testing targets so we are consistent with Core team's implementation.

cacheung · 2024-02-05T18:47:36Z

.circleci/config.yml

          #   The test command
-          test-command: make ci-functional-test
+          test-command: make functional-test


Do you want to change functional-test-coverage now or later?

cacheung · 2024-02-05T18:59:15Z

Makefile

 	(./code/gradlew -p code/${EXTENSION-LIBRARY-FOLDER-NAME} publishReleasePublicationToSonatypeRepository)

-ci-publish-main: clean build-release
+publish-main: clean assemble-phone-release


Userprofile keep them as
ci-publish-staging
ci-publish

Should we follow to make it standard?

…d update report locations in CircleCI config.

… in Makefile

…Cov report.

…ables set by CI.

cacheung · 2024-02-07T00:15:49Z

Makefile

-ci-generate-library-release:
-		(./code/gradlew -p code/${EXTENSION-LIBRARY-FOLDER-NAME}  assemblePhoneRelease)
+assemble-phone-debug:
+	(./code/gradlew -p code/${EXTENSION-LIBRARY-FOLDER-NAME}  assemblePhoneDebug)


Do we use {EXTENSION-LIBRARY-FOLDER-NAME}
Or
(EXTENSION-LIBRARY-FOLDER-NAME)

GNU allows both for variable expansion. However we should be consistent, plus some have reported weird errors when mixing both types in the same line. Let's stick with using $() instead of curly braces.

kevinlind added 2 commits February 4, 2024 21:00

Remove 'ci' folder and 'ci-' prefix from Makefile targets

b9c2f33

Remove Makefile step to rename .aar file.

dfe87fb

kevinlind commented Feb 5, 2024

View reviewed changes

kevinlind requested review from cacheung, emdobrin and timkimadobe February 5, 2024 05:20

cacheung reviewed Feb 5, 2024

View reviewed changes

kevinlind added 4 commits February 6, 2024 14:10

Add separate 'unit-test' and 'unit-test-coverate' Makefile targets an…

b1791c5

…d update report locations in CircleCI config.

Add separate 'functional-test' and 'functional-test-coverage' targets…

22dd52d

… in Makefile

Reorder CircleCi steps to save tests/artifacts before publishing Code…

e6f5522

…Cov report.

Add 'ci-' prefix to publish targets as these targets require env vari…

e637d31

…ables set by CI.

cacheung reviewed Feb 7, 2024

View reviewed changes

Use parenthesis for Makefile variable expansion.

920d254

cacheung approved these changes Feb 7, 2024

View reviewed changes

kevinlind merged commit c449867 into adobe:dev-v3.0.0 Feb 7, 2024
7 checks passed

kevinlind deleted the remove-ci-folder branch February 7, 2024 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Makefile and CI scripts to remove 'ci' folder and 'ci-' prefix. #130

Update Makefile and CI scripts to remove 'ci' folder and 'ci-' prefix. #130

kevinlind commented Feb 5, 2024

kevinlind Feb 5, 2024

kevinlind Feb 5, 2024

kevinlind Feb 5, 2024

kevinlind Feb 5, 2024

cacheung Feb 5, 2024

kevinlind Feb 5, 2024

kevinlind Feb 6, 2024

codecov bot commented Feb 5, 2024 •

edited

cacheung Feb 5, 2024

kevinlind Feb 5, 2024

cacheung Feb 5, 2024

kevinlind Feb 5, 2024

kevinlind Feb 6, 2024

cacheung Feb 5, 2024

kevinlind Feb 5, 2024

cacheung Feb 5, 2024

kevinlind Feb 6, 2024

cacheung Feb 5, 2024

kevinlind Feb 6, 2024

cacheung Feb 5, 2024

cacheung Feb 7, 2024

kevinlind Feb 7, 2024

		(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) lint)


		ci-build: create-ci

Update Makefile and CI scripts to remove 'ci' folder and 'ci-' prefix. #130

Update Makefile and CI scripts to remove 'ci' folder and 'ci-' prefix. #130

Conversation

kevinlind commented Feb 5, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

codecov bot commented Feb 5, 2024 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 5, 2024 •

edited